I don't understand this one:

     -                (written_out - i) * sizeof *pDecoded);
     +                (written_out - i) * sizeof(*pDecoded));

Edit: just to be clear-- I'm claiming there's no way the addition of parentheses could change the behavior of the program.

I guess I can understand resolving an ambiguity so that the reader doesn't have to look up precedence rules. But when combined with a high-profile bug in a critical piece of infrastructure it looks a bit like the software equivalent of not shaving one's beard in the hopes of winning a sports tournament.

Looks to me like a coding style fix along for the ride with the correctness fix.

I don't think that's security-relevant, likely just keeping a linter happy.

Ah, I didn't think about a linter. I'll take a look at the code later to see if that's the case.

sizeof is a keyword because of some terrible reason, but I'd parenthesize it just to make the intention clear.

Agreed. sizeof FEELS like a function, and so I use parentheses with it, even though I know it's not actually a function.

Wasn't there some subtle difference between using sizeof with vs without parens? Something like if you pass a type to sizeof -- e.g. sizeof(*int) -- you must use parents anyway? It's been years since I've used C, I'm probably not remembering it correctly.

Yes, exactly that. `sizeof(int)` OK, `sizeof(i)` OK, `sizeof i` OK, `sizeof int` not OK. One of those weird rules.

Why is sizeof a terrible keyword? Just think of it as an operator rather than a function.

Should have been a callable, or should have had consistent syntax to prevent people from being clever.

From the variable names, it is clear that the old version was correct. Someone should revert this. :-)

That diff hunk, SMDH. It's incredible what C programmers will do just to avoid using a real language. The whole project is toxic waste.

I don't think it is fair to blame the language for this.

Look at the lines above the vulnerability:

  n = n + i / (written_out + 1);
  i %= (written_out + 1);

Whatever buffer manipulation this is doing could be abstracted out into a library. (As studio.h does.)

Also, since written_out probably varies from loop iteration to iteration, what does "n remainder i" even mean?

Also, why don't they increment written_out before doing the arithmetic, and why do they increment i on line 523, when the only intermediate use of it is "i + 1"?

I've written some crappy code in my time, but when things descended into that level of insanity, I either delegated to fuzz tested functions, or at least left a comment.

You’re not wrong.

Back in the day, you didn’t have alternatives with zero-cost abstractions like we have today. There are many valid reasons why one would make decisions to balance performance against memory-safety, especially 25 years ago – and given the fact that networking code tends to be part of hot paths where optimization matters.

come on, that is quite harsh. c is easily my favorite programming language and it just works really damn well and produces very small binaries. it's easy to write terrible code in any programming language.

> c is easily my favorite programming language

That's not really relevant to the discussion of whether it's the proper language to be used for something like OpenSSL.

> it just works really damn well

As far as I can tell, all mainstream programming languages "work well", so it's not clear what you mean.

> produces very small binaries

True but not very important IMO

> it's easy to write terrible code in any programming language

Sure. But it's not easy to write this particular flavor of terrible code -- buffer overflows -- in any commonly-used language other than C and its derivatives.

C++ generally produces smaller and faster programs because the language gives compilers much more to work with.

To be fair, OpenSSL was first released in 1998. We didn't have fast memory-safe languages back then. Java existed, but was incredibly slow.

The function in question was written in 2020. OpenSSL 3 is an incompatible major rewrite undertaken during 2018-2021 and there is no no valid excuse for why language quality was not improved at that time.

> OpenSSL 3 is an incompatible major rewrite

Oh snap I didn't know that, and I probably should have. TIL.

Then yeah, you're right. It probably should have been written in Rust.