It's documented as returning 1 when verification succeeds and 0 when it fails, so this appears to turn an error into a failure to verify, which seems to be the intent?
Right. Previously the return code from x509_verify_cert was returned directly to the caller, which was incorrect - ssl_verify_chain_cert was supposed to return -1 if the caller should retry, but would end up doing so on any error that was returned from x509_verify_cert. So the patch is correct.
If your argument is that the return codes should be consistent across different layers of OpenSSL then I'm not going to disagree, but this is a large old codebase and this isn't some unique case.
Siblings have said this is fine and I agree, but I agree w/ you that the intent is obscured. It looks like a clamp or a hack. I think it'd be clearer to assign to a different variable? Ex:
int vr = X509_verify_cert(ctx);
if vr <= 0 {
i = 0;
} else {
i = 1;
}
But also, like another commenter suggested, using enums or #define would be helpful documentation too.
And finally, i isn't a wonderful name for this variable. rv or ret or status are all better options.
For some reason, bad naming conventions are baked into the C programming culture. Like you aren’t a real C programmer if you don’t use two letter variables. I’m assuming short names and schemes like Polish notation once had a place when RAM was more expensive, but I can’t comprehend why new C code can’t use modern practices.
> Like you aren’t a real C programmer if you don’t use two letter variables.
Reasonable people disagree about this. John Ousterhout is in the "use meaningful variable names" camp, and Rob Pike is in the "use short variable names when the meaning is obvious" camp.
The reason I think "i" is bad here is that "i" usually (almost always?) means "index". You could return "i" if you were searching for the index of something, but that's not what this function is doing.
A lot of bugs have this pattern of layers of incongruity. "i" is usually an index, but here it's the return status value. These two sections of the library use different return status value semantics. You can kind of think about it like steering a ship through icebergs of inconsistency--eventually you're gonna hit one.
There are things you can do, and they all exist on the continuum of "easy/ineffective - hard/super effective". This function could use enum/#define. As a sibling comment said, if OpenSSL used typedef'd enums for its status codes the compiler would complain, and then you can use switch/case (maybe even wrapped in a conversion function) to convert between different universes of status codes. You could completely separate these sections of the library, which are currently very intermingled, and communicate between them using specific interfaces.
As an example for that last one, this code doesn't care about various error conditions when it comes to cert verification, it just wants to know if the cert checks out or not. So when X509_verify_cert returns more than 2 possible values, it's leaking irrelevant information. The X509 section could have a different function that returned a boolean, specifically for uses that don't care about the details of certificate verification failure.
And actually, it looks like every single use of X509_verify_cert only checks for > 0. Nothing cares about 0 vs. < 0. Was it ever important for something external to know if there were internal errors vs. the certificate failed? Is that even a good idea at all?
These are the kinds of things you look into and think, "what is going on here". Evidence starts mounting that like, this thing isn't built very well (or this gem [1], the product of a code formatter, making one worry about potential other goto fail problems)
