6 ms·
The code is crap and while I can understand why a single person might have written it this way, I think any organization of full-time professional software deve
by thrownaway2424 13y ago
The code is crap and while I can understand why a single person might have written it this way, I think any organization of full-time professional software developers should be collectively embarrassed to have accepted it. The only reason to use the "goto fail;" idiom is to free two buffers before returning. But the buffers in question are just sslBuffer structs that live on the local stack. Their destructors will be called, in reverse order, regardless of how the function is exited. If this code simply had a fucking destructor for sslBuffer to delete the data pointer, none of the rest of these call sites for SSLFreeBuffer would need to exist at all! And there's 24 such call sites in this file alone. Anyone with any sense would chose the 2-line destructor over dozens of calls to free buffers.
There's even this bullshit:
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
goto fail;
Which is hilarious, because the only way for SSLFreeBuffer to fail is if the buffer doesn't exist, but hashCtx is a local temporary object and it MUST exist. Anyway the only thing we do by jumping to fail is to free the non-existent object once again.
In short: wow.
- _pmf_ 13y ago> Their destructors will be called, in reverse order, regardless of how the function is exited. Not in C.
- thrownaway2424 13y agoYes, one makes a conscious choice of implementation language. They aren't dictated by the laws of physics. Choosing your implementation language is a rather important step.
- andrewflnr 13y agoIndeed, a step involving more factors than simply whether the language has destructors.
- kabdib 13y agoNope, it's actually pretty decent C. With a bug.
- beachstartup 13y agothis pattern is structured to avoid the use of an additional stack (local) variable to track a condition, and to skip superfluous execution of code once that condition is detected. i've used it myself to optimize inner loops of very simple un-accelerated graphics rendering code (which sped it up considerably, since it is potentially skipping many levels of unnecessary execution on the cpu, millions of times), but i agree with some posters here, using it in a security context like this is a bit daring. still, having said that, this bug really is an indictment of the testing process and not really bad style per se.
- kabdib 13y agoThe code already has that condition variable ('err'), the goto is to avoid execution of code that should not be executed. It's not about avoiding additional stack -- that was probably the last thing on the programmer's mind. The idiom is about error handling.
- beachstartup 13y agoi didn't see that. that makes it even worse. this design is supposed to avoid that extra var.