Fix network usage - #433
Conversation
Reported by: Claude Sonnet 5 Bug bounty: $30 (3 * $10 "harmless")
If close() failed, we would re-enter drop() and then call
network_read_cancel() on an already-cancelled cookie. This fixes it by:
- setting the cookie to NULL
- ensuring that we remove the node from the list, even if close() fails.
Reported by: Claude Sonnet 5
Bug bounty: $10 ("harmless")
The bare free(A) ignored A->conn_cookies.
Reported by: Claude Sonnet 5
Bug bounty: $10 ("harmless")
|
This follows up to Tarsnap/libcperciva#543. |
| network_accept_cancel(A->accept_cookie); | ||
| err3: | ||
| free(A); | ||
| simple_server_shutdown(A); |
There was a problem hiding this comment.
Is this safe to call on all possible event-loop exits?
There was a problem hiding this comment.
Haven't seen a problem yet, but I'll return to checking it tomorrow.
3ea12c0 to
192f100
Compare
Also, set a cancelled cookie to NULL. This isn't strictly necessary;
it's is part of the clean-up so we'll be freeing the whole struct soon
anyway, but it looked a bit questionable.
Reported by: Claude Sonnet 5
Bug bounty: $11 ("harmless" + "style")
The original design of the server was to quit on any type of failure;
it's intended for single-machine testing where something like ECONNRESET
is unlikely.
However, we might as well relax that assumption, and keep the server
going even if a client is misbehaving.
Reported by: Claude Sonnet 5
Bug bounty: $10 ("harmless")
192f100 to
3b97d17
Compare
|
Revised with 2 new commits, although I'd like to change the order before the final merge. In particular, move the |
| A->accept_cookie = NULL; | ||
| } | ||
|
|
||
| /* Stop drop() -> conndied() -> doaccept() from arming a new accept. */ |
There was a problem hiding this comment.
I'd put this above the /* Cancel any further attempts. */ block; we're single-threaded so there isn't actually any race, but it's cleaner in principal to say "don't restart this, and stop it now" rather than "stop it now and don't restart it".
|
s/it's is part/it's part/. |
|
What's the "NRC" in "network_read_cancel(NRC)"? |
| *(P->status) = -1; | ||
|
|
||
| /* Inform the upstream that our status has changed. */ | ||
| P->callback(P->cookie); |
There was a problem hiding this comment.
(P->callback)(P->cookie); is our normal style for this.
|
A few minor nits, but I think the code is right in concept now. Yes, please re-order commits to avoid introducing a bug and then fixing it. |
network_read_cookie. I'll reword it. |
… in shutdown
Also, set a cancelled cookie to NULL. This isn't strictly necessary;
it's part of the clean-up so we'll be freeing the whole struct soon
anyway, but it looked a bit questionable.
Reported by: Claude Sonnet 5
Bug bounty: $11 ("harmless" + "style")
We assigned `node_ptr`, but only used it for the assert().
This should have been part of:
2022-04-17 tests/simple_server: switch to LIST from queue.h
7be2eca
Suggested by: Claude Sonnet 5
|
Next edition:
I haven't re-ordered the commits yet, because that would spoil the attempt to only show changes since your previous review. |
2 participants
No description provided.