remove panics per PanicAndRecover guidance from go authors #24
Conversation
|
I'm aware of the guidance, all of these panics are in areas that I understood to never error under normal operating conditions. I think the only one that can actually occur is |
|
Looks like the diff page half-rendered, I do see a few in the protocol implementation that would make more sense to change. |
|
Take a look here for a bunch of similar examples: https://github.com/golang/crypto/search?q=panic Instead of removing all of them, what do you think about just identifying the ones that do not indicate a problem with the calling code and instead indicate an error that could reasonably be triggered by user input? |
|
That's a fair point, re: these shouldn't ever happen. I've encountered them only with ReadMessage and WriteMessage, and the issues were 100% my fault. That said, I'd much prefer the calling code had chance to gracefully handle an error. The panic caused the program to terminate, which exacerbated a handshake bug by terminating a daemon and triggering even more handshakes. Just to reiterate: I'm not saying this is the fault of the lib, but this was a particular sharp edge that made for a cascading failure because of our daemon exiting. |
cipher_suite.go
Outdated
| @@ -216,7 +216,7 @@ var HashBLAKE2b HashFunc = hashFn{blake2bNew, "BLAKE2b"} | |||
| func blake2sNew() hash.Hash { | |||
| h, err := blake2s.New256(nil) | |||
| if err != nil { | |||
| panic(err) | |||
| return nil | |||
There was a problem hiding this comment.
Let's keep everything else but revert these return nils as I think they will either panic 100% of the time or never.
There was a problem hiding this comment.
Will do. I'll update the PR shortly.
Makes sense, once we address the nil returns from the constructors and get the tests passing I think we can merge this. |
|
PR updated. Let me know if there is anything else you'd like me to consider. |
|
Looks like the tests are failing because vectorgen no longer compiles, do you mind fixing that? |
|
I see I broke vectorgen. fixing now |
|
All set. Thanks again for your help! |
|
I think it's a little unsafe for key generation functions to return an error because of the risk that someone ignores the error (who ever expects keygen to fail?) and ends up using an all-zero key. Are there any safety checks elsewhere in the library that prevent an all-zero key from being used? |
This is a good point. There are no checks, the ed25519 package will blindly use whatever bytes are passed in. However, Go programmers are pretty good about checking errors, and the ephemeral key generation will return without using a zero key, so we're just talking about static key generation outside of the handshake. @rawdigits How do you feel about leaving the panic in? I think the most likely failure mode would be running out of file descriptors on a kernel without |
|
I would definitely prefer to bubble up the error and not panic here, considering it still might be triggered unexpectedly by underlying OS concerns. The handshake already deals with this afaict, and this method could almost be private, but I'd still really prefer to not panic here unless you think it is strictly necessary. |
|
Thanks @rawdigits! |
Using this as a lib is a bit sticky due to the number of built in panics, which would require some often-called methods to be caught by rescue in an anonymous function within a defer. This is not ideal for performance and generally panics in libs go against the guidance of the go creators:
From https://github.com/golang/go/wiki/PanicAndRecover
"By convention, no explicit panic() should be allowed to cross a package boundary. Indicating error conditions to callers should be done by returning error value. Within a package, however, especially if there are deeply nested calls to non-exported functions, it can be useful (and improve readability) to use panic to indicate error conditions which should be translated into error for the calling function. Below is an admittedly contrived example of a way in which a nested function and an exported function may interact via this panic-on-error relationship."
There is a valid case for the lib to panic if the user takes over the cipher state as an extra sanity check before they do something potentially catastrophic with counters.