Even though the code got better with this fix I still wouldn't accept code that looks like this in a review. Why are 1, 2, 3, 16 not defines? What's up with the code duplication between files? Where are the unit-tests?
I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software.
> I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software.
I get the impression that this applies to openssl far more than other software. The code base is a mess, and it's security sensitive. So people dare not touch it.
It's a shame that there isn't a better incentive for this particular code base to be fixed.
Yep, and no braces after the 'if' statement in the patch. Even after the previous ssl vuln (I thought it was gnutls- struggling to find the relevant hn discussion) was caused by an omission of braces after the 'if' statement.
You are thinking of "goto fail", a bug in Apple's Security framework. I would not claim that was "caused" by a lack of braces: even having the braces, that bug--in addition to a wide class of similar bugs--is still quite possible, even if in a few models of how the bug was caused it becomes slightly less likely. The best place to lay blame for that kind of error is a stubborn insistence that error handling should involve boilerplate return value checks strewn throughout the code, with no attempt at abstraction or structure: it leads to numerous potential mistakes. Please read the various discussions attached to this article that made this claim:
I felt much better about having a fix before I looked at the actual code. It is functionally secure (looks right to me and thousands of others by now), but the way it is written would guarantee you an instant fail in an exam or interview.
Even though the code got better with this fix I still wouldn't accept code that looks like this in a review. Why are 1, 2, 3, 16 not defines? What's up with the code duplication between files? Where are the unit-tests?
I'm starting to feel that a lot of software that has been around for 10+ years and is commonly used does not live up to current best practices regarding writing good system-level software.