Guard StreamEngine handshake against null peer identity when RecvIdentity is enabled#1160
Conversation
Agent-Logs-Url: https://github.com/zeromq/netmq/sessions/f5cde7c6-6d90-4da1-9926-4e0396ec2b03 Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com>
| Msg identity = new Msg(); | ||
| identity.InitPool(m_mechanism.PeerIdentity.Length); | ||
| identity.Put(m_mechanism.PeerIdentity, 0, m_mechanism.PeerIdentity.Length); | ||
| byte[] peerIdentity = m_mechanism.PeerIdentity; |
There was a problem hiding this comment.
@copilot is this code not null annotated? If this can be null, then the old code should ideally have flagged a warning. Is it feasible to annotate it as part of this work?
There was a problem hiding this comment.
Good point. I updated this with focused nullable annotations in StreamEngine.MechanismReady by enabling nullable context for that method and changing peerIdentity to byte[]?, so the potential null is explicit in the code path (f841e0c). Mechanism.PeerIdentity was already nullable (byte[]?).
Agent-Logs-Url: https://github.com/zeromq/netmq/sessions/bfe457f8-b95d-4df4-8cbc-0e45caa5cf65 Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com>
|
@copilot in what case would |
|
|
@copilot PTAL at the macos failure. Even if it's unrelated, see if we can fix it. |
Agent-Logs-Url: https://github.com/zeromq/netmq/sessions/d30df06d-eaa6-46d7-b83a-7574408492b7 Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com>
I tracked the macOS failure to a hang in |
…imeout to stress the race Agent-Logs-Url: https://github.com/zeromq/netmq/sessions/6a257730-950c-4974-a634-78c12e549d5b Co-authored-by: drewnoakes <350947+drewnoakes@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1160 +/- ##
===========================================
- Coverage 65.98% 19.73% -46.25%
===========================================
Files 145 148 +3
Lines 9149 9408 +259
Branches 1486 1544 +58
===========================================
- Hits 6037 1857 -4180
- Misses 2500 7270 +4770
+ Partials 612 281 -331
🚀 New features to boost your workflow:
|
Remove #nullable disable and annotate all nullable types: - m_handle, m_session, m_socket, m_ioObject are lifecycle-nullable - m_encoder, m_decoder, m_mechanism are nullable until handshake - m_inpos, m_outpos are nullable buffer positions - Use null-forgiving operator where state machine invariants guarantee non-null (post-Plug, post-handshake, post-V3-handshake) - Remove redundant null assignments in constructor for nullable fields This is the file that motivated the nullable annotation effort (PR zeromq#1160). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove #nullable disable and annotate all nullable types: - m_handle, m_session, m_socket, m_ioObject are lifecycle-nullable - m_encoder, m_decoder, m_mechanism are nullable until handshake - m_inpos, m_outpos are nullable buffer positions - Use null-forgiving operator where state machine invariants guarantee non-null (post-Plug, post-handshake, post-V3-handshake) - Remove redundant null assignments in constructor for nullable fields This is the file that motivated the nullable annotation effort (PR zeromq#1160). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TimeSpan.FromMilliseconds(1)in reader loop so the lock is held long enough to stress the concurrency racecatch (TerminatingException) { }to the reader task — the actual bug fix (this exception escapes when the socket is disposed whileProcessCommandsis in progress)TerminatingExceptionmust be caught