Hacker News Reader

Comment by erdeszt | Hacker News Reader

Here's the referenced commit for the interested: https://github.com/libressl-portable/openbsd/commit/ddd98f8e...

To be honest I'm kinda surprised that even after the 'goto fail' story people still write code in this questionable style(I know this particular issue is not stemming from the lack of curly braces, but still).

View on HN

surprised that even after the 'goto fail' story people still write code in this questionable style

LibreSSL didn't spring into existence out of whole cloth. It started as a fork of OpenSSL, which goes back to 1998.

The "questionable style" is from legacy code. It would be a massive effort to revise the entire codebase. And if LibreSSL did that, it would make it harder to import changes from OpenSSL and from other forks such as BoringSSL.

> it would make it harder to import changes from OpenSSL

When it's a choice between making it easier to import changes or harder to import bugs, I know which one I think is more important when dealing with a security library.

I don't think you see the amount of work that would go into refactoring a project of this scale whilst keeping up with the latest patches from upstream.

I do understand the the amount of work, but that wasn't the only argument presented. It was also presented as making it harder to accept patches. Since one of the goals of LibreSSL is to start applying security best practices, and one of the reasons for its existence is the poor quality and recurrent problems with OpenSSL, keeping compatibility with OpenSSL to make it easier to accept patches should be very low on the list of priorities.

Put another way, if you forked because upstream was crap, not changing because it makes it easier to accept upstream patches is a poor reason not to change something that might benefit from it.

I tend to agree, maybe with one exception. Would the end result of a LibreSSL refactor introduce more bugs than leaving the current process in place (with improvements)? It seems like we can't know the answer to that question until it happens so speaking definitively about either option becomes questionable.

Definitively, no, but the lack of of a definitive (or even likely) answer still makes the it a poor choice, if not in the sense of "wrong" than in the sense of "this is something that needs to be figured out based on your goals, and until you do choices based on this will not be grounded on fact" so it's poor in that it's not well grounded.

That said, the relatively poor history of OpenSSL and the relatively high quality of software that comes out of the OpenBSD project leads me to think I know what the likely outcome of refactoring code is in this particular scenario.

That is certainly true and perhaps another advantage exists from a refactor of LibreSSL: Finding unknown bugs in OpenSSL that may only be revealed during refactoring.

> it would make it harder to import changes from [...] BoringSSL.

BoringSSL doesn't use that style: https://boringssl.googlesource.com/boringssl/+/master/ssl/s3...

There's no need to revise the entire codebase either, just fix the lines you touch, and don't introduce new cases like in that commit.

The code style in question is the current recommendation in the systemd style guide - a much more current project, and a rule that was put in place in 2014.

https://github.com/systemd/systemd/commit/61f33134fc9231e07e...

I'm all for blaming legacy but this is unfortunately still a trend.

I feel like having "safety net" sections is a smell, too. Maybe it's due to limitations of the language? I'm not a C expert.

Safety nets make it seem like you're handling edge cases in a vaguely specified, ad-hoc way, which is prone to forgetting to add the safety net in at least some places, while the nets themselves are easy to mess up as well.

Could this code benefit from more typing perhaps? Automated checking of pre- and postconditions? Are there C (macro) libraries implement that in a usable way?

Yeah, the real smell here is the "safety net" which is also what seems to have caused the bug. Clean quality code should avoid this kind of safety nets as much as possible and instead make it hard in the first place to get the program into an invalid state and if we do get into an invalid state not just silently try to guess what the user really wanted to do. As it turns out people actually wanted to return success with an error code and relied on this being possible in their applications.

A better type system would help out a lot here, but it is also possible to write clean C code without this class of bugs. And OpenSSL has some of the worst code I have seen in an open source project (I have seen much worse in commercial projects), so while C has a lot of flaws do not judge it after OpenSSL.

"safety net" cases are pretty much covered by design-by-contract 101.

I think there is a misunderstand of what the existing safety net is about. There are 2 error states: did the verification fail and should the connection be aborted. The safety net makes sure that if a function (the callback) says the connection must be aborted but didn't set the verification error, that it sets an unknown verification error. Note that the callback is external code.

The new "safety net" that libressl added said that if the connection doesn't need to be aborted there was no verification error.

This is C. There's nothing questionable about this "style." It is just how C is written. How else are you going to free resources without a code section to do it? Copy/paste it dozens of times?

The specific code isn't "goto fail" anyway, it is more akin to a finally block.

I was referring to the lack of curly braces around the body of the then clause. This code has nothing to do with resource management and gotos per se so I don't see where your frustration and attack comes from.

If you compile your code with -Werror -Wmisleading-indentation it's no more dangerous than writing Python.

I think erdeszt is just referring to the lack of curly braces around the statements governed by the if statements. This can be done around any single-line statement block, and is generally safer in the face of merges from version control.

Reply on news.ycombinator.com