proto_handshake: fix clean-up on mkkeys() failure - #427
Conversation
80b55bb to
cd0985b
Compare
| /* Clean up. */ | ||
| if ((rc = handshakefail(H)) == 0) { | ||
| /* If there's no error in the clean-up, set a default error. */ | ||
| rc = -1; |
There was a problem hiding this comment.
I'm less confident about this than I am most of the other recent PRs -- why don't we want handshakefail() to return -1 in more cases?
There was a problem hiding this comment.
Yeah this bit isn't right. An error from proto_crypt_mkkeys means "we were unable to generate keys because we ran out of memory"; it's a fatal error. We call handshakefail in many non-fatal code paths, e.g. if the network dropped before we got DH parameters, so in the common case it will return 0, and your patch will mask the "we should exit rather than just dropping this one connection" fatal error from proto_crypt_mkkeys.
This should just be
err1:
handshakefail(H);
/* Failure! */
return (-1);
|
|
||
| err0: | ||
| err1: | ||
| /* Clean up, but we don't care if the callback succeeded or failed. */ |
There was a problem hiding this comment.
Is it worth adding a comment about this? Or mention something about informing the caller?
There was a problem hiding this comment.
No, we have a lot of "we're cleaning up from an error, there's no point checking if we had another error" paths. This might be an idiom which is worth documenting in STYLE I guess ("don't check for for failures in an error-handling path if they will not affect behaviour")? But there's definitely no point documenting it every time it happens.
159669c to
d2e406d
Compare
Reported by: ChronoNova
Bug bounty: $10 ("harmless")
|
Rebased |
2 participants
No description provided.