2

In light of the recent OBJ_obj2txt vulnerability in LibreSSL (which was found during the OpenSMTPD audit, and does not affect OpenSSL), it came to my attention that the memory leak issue likely resulted from some earlier code refactoring, where the block scoped variable char *bndec was moved out to be function scoped instead.

I know first-hand that there is this great resistance to block scoped variables within old-school projects like OpenBSD, but what other justification would there be to move the char *bndec declaration?

More broadly, when was block scoping introduced for variables in C? All I could find is that it was already part of C89. Is that where it started, or was it also part of an earlier spec?

  • @cnst: Sounds like you already know the answer to your own question. Perhaps you can provide proof for your assertion: "Frankly, I know first-hand that there is this great resistance to block scoped variables within old-school projects like OpenBSD," as there seems to be no such resistance apparent here. – Robert Harvey Oct 19 '15 at 7:09
  • 1
    @RobertHarvey, but I don't know the answer; just because your answer is wrong (wasn't me who pointed it out! But I did agree!), doesn't mean I have a better one. – cnst Oct 19 '15 at 7:21
  • 3
    Have a look at programmers.stackexchange.com/q/200709, especially the accepted answer: "If your function is so long that you cannot recognize any unwanted side effects or illegal reuse of variables any more, then it is time to split it up in smaller functions - which makes an internal scope pointless." – Robert Harvey Oct 19 '15 at 7:28
  • Why did this question attract so much attention, yet has such a low score? – cnst Oct 21 '15 at 3:47
7

TL;DR: It's not an issue of block scoping, but rather a misunderstanding of how pointers should be handled.


Courtesy of the vulnerability listing that you posted, let's take a look at the code in question.

489 int
490 OBJ_obj2txt(char *buf, int buf_len, const ASN1_OBJECT *a, int no_name)
491 {
...
494         char *bndec = NULL;
...
516         len = a->length;
...
519         while (len > 0) {
...
570                         bndec = BN_bn2dec(bl);
571                         if (!bndec)
572                                 goto err;
573                         i = snprintf(buf, buf_len, ".%s", bndec);
...
598         }
...
601         free(bndec);
...
609 }

And if we delve into the declaration of BN_bn2dec we see a comment of:

/* Must 'free' the returned data */

What you see as a block scoping issue, I see as a poor understanding of pointer operations as the root issue.

This diff revision shows when the *bndec was moved from inside the while loop to outside the while loop. The error was also introduced at the same time when the free(bndec) statement was moved outside the loop.

Where *bndec is declared for this function is a bit irrelevant. It can safely be defined either outside or inside the while loop. And if memory serves correctly, the actual declaration will be moved to the top of the function by the compiler anyway.

Declaring *bndec inside the loop merely would have forced an error if the free() call remained outside of the loop. It remains to be seen whether that error would have been sufficient to alert the programmer to the fallacy behind their refactoring logic.

Being a bit of a pessimist when it comes to most programmers and pointers, I'm going to posit that the free accessing an out-of-scope variable error wouldn't have helped them discover the error of their ways. Instead, they would have taken the naive (and incorrect) solution of resolving the error by merely moving the variable declaration outside of the block. And that's why I disagree with the contention that the root cause of this is a block scoping issue.

If C happened to have garbage collection, then block scoping would have been of more value in this case. But it doesn't, and that line of thought becomes pointless speculation.


Unfortunately, we don't really know why the refactoring programmer chose to move that free(bndec) statement. All that they left via check-in comments was this:

Clean up some of the nightmare of string and pointer arithmatic in this nasty function.
This gets rid of the nasty tmp variables used to hold temporary strings and the DECIMAL_SIZE hack. it gets rid of the rather pointless null checks for buf (since the original code dereferences it before checking). It also gets rid of the insane possibility this could return -1 when stuff is using the return values to compute lengths All the failure cases now return 0 and an empty string like the first error case in the original code.

And while I respect their dedication to clean code, I'm not certain I fully agree with their removing of various checks from the code. Having written a fair amount of C code with a fairly large team of developers, my experience is that defensive coding practices are almost always warranted. And while a developer may not be able to justify the time expense to add those defensive practices, I can't say that I have ever found valid grounds to remove those defenses.

To delve into an equal amount of hyperbole as the refactoring programmer, I find their check-in comment to smack a little bit too much of hubris and not enough of humility. That hubris is likely what led them to assume they "knew what they were doing" (TM) in moving the free(bndec) statement, and they didn't mentally walk through what that was going to do to the code.

This error was introduced with the refactoring not because of the block scoping, but because of the failure to understand how pointers operate and the failure to understand how pointers relate to memory.


While I think I have established why block scoping is irrelevant to this issue, I want to address what you see as "great resistance to block scoped variables within old-school projects". And to my knowledge, ANSI C has always had block scoping available for variables. Going out on a limb, I suspect K&R C also had it.

Keep in mind that C is not a gentle language for the naive or the initiate. It was created in order to provide an additional layer of abstraction over B and assembly level programming. Programmers writing in C were still expected to have an understanding of the underlying assembler that was being generated. For example, variables are not automatically initialized, and many a programmer has been burned by that lack of initialization. C expects the programmer to understand what that variable declaration maps to in memory. C also has more than its fair share of unwritten style rules that are considered to be "good form."

Moving all variable declarations is one of those unwritten style rules, and the pragmatic reason goes back to the lack of automatic initialization. Disciplined C programmers declared all of their variables at the beginning of the function so they could make a quick visual scan to verify that all declared variables had been initialized. It's an easy way to make sure that a common mistake (e.g. lack of initialization) has been avoided.

| improve this answer | |
  • 1
    That's the danger with functions that allocate code and make the caller responsible for freeing it. IMO the better solution would have been to let the caller pass in a buffer and its size to get filled with the hex of the bignum. – ratchet freak Oct 19 '15 at 15:06
  • 1
    "And if memory serves correctly, the actual declaration will be moved to the top of the function by the compiler anyway.": At least this is what gcc does (just checked it with a small example): variables declared in a block are allocated at the beginning of the function along with the other variables. – Giorgio Oct 19 '15 at 17:51
  • 1
    @Giorgio, but you're talking about the final compiled low-level assembly / machine code code, right? Because I'm pretty sure that at the higher level, even if such optimisation is done at the final stage (and I'm sure you're correct and it is), an undefined variable would still have to be reported as an unrecoverable error should there have been an attempt to free a block scope variable outside of the block it was defined in. – cnst Oct 20 '15 at 3:57
  • 1
    @cnst block scoping is irrelevant because the naive solution resolving the error would have been to simply move the pointer declaration outside of the block. You're assuming that the programmer making the change would have seen the error and said "Oh, I'm an idiot, I just created a memory leak!" My experience with developers making changes like the one shown in the diff is that they would not have understood what they were doing and taken the naive solution instead. The problem is in moving the free, and there was no grounds to do so. – user53019 Oct 20 '15 at 13:44
  • 1
    @PeterGreen: I try to avoid declaring a variable in the middle of a block. I prefer the structure <declare and set variables> <compute something and return>, a bit like writing a let* in Lisp or a let in Haskell. – Giorgio Oct 20 '15 at 15:39
5

In the C Reference Manual that came with 6th Edition Unix (May 1975, second volume entitled "Documents for Use With the Unix Time-sharing System") the only compound statement allowed to contain variable declarations is the function-statement:

function-body: type-decl-list function-statement

function-statement: { declaration-listopt statement-list }

In the first edition of K&R ("The C Programming Language", 1978) you can read:

Declarations of variables (including initializations) may follow the left brace that introduces any compound statement, not just the one that begins a function

This specification is a hint that block scoped variables was probably a recent, not well known feature (1978).

See also Dennis M. Ritchie's page

| improve this answer | |
1

I believe it's originally there in the first place. The {} are not just part of control structure syntax or function definitions, but they also define a compound statement (block).

It's mentioned in this page: "You can use a block wherever the grammar of PDP-11 C requires a single statement."

So, the compiler did extra work when dealing with them, it's not just used to know where does a block start and end.

Edit to clarify context

From wikipedia, "The development of C started in 1972 on the PDP-11 Unix system".

| improve this answer | |
  • This seems interesting, but it's not really an answer at all -- how does your information compare to programmers.stackexchange.com/a/300274/89124, for example? You cite no year, no context (sorry, I don't happen to be a PDP-11 expert) -- this ain't an answer at all. – cnst Oct 21 '15 at 3:34
  • sorry, I'll clarify the context – imel96 Oct 21 '15 at 5:51
  • That's only a bit better -- aren't you now claiming that it wasn't introduced to C before C itself was created? :-) – cnst Oct 21 '15 at 7:32
  • I only try to answer when block scope was introduced (it wasn't) and I don't think it's bad. In fact, C++ does rather similar thing with allowing declaration in 'for' loop. – imel96 Oct 21 '15 at 13:20
  • declaration within the for loop is legal in C99, too; as for your answer, no, you're not answering when it was introduced, because you never attempt to establish the last-modified date for the quote or document you reference; for all we know, PDP-11 C has been supported and updated for years, and the document you reference might as well be from 1999! – cnst Oct 22 '15 at 5:57
0

This is going back 25 years, but I can remember the limits of screens where you could only see 20-22 lines of code on the screen at one go (due to terminal screen size to the Unix main frame) and it was very easy to have a function variable and a block variable with the same name and not realise it. gcc brought in a --shadow flag to warn about this, but not all compilers had an equivalent.

Another issue was with code readability.

int j;
for(j=1;j<10;j++)
{
    int i=5;
    i++;
    printf("i=%i\n",i); 
}

Many developers at the time seemed to read this loop as setting i=5 every time around the loop, and not just set when the variable was created. Having

static int i=5;

declared within a loop would have caused no end of confusion.

| improve this answer | |
  • 4
    Ugh... { int i=5 is setting the variable to 5 every single loop. But, yes, static int within a block is indeed confusing. I've blogged about it a few years ago -- cnst.livejournal.com/30259.html. – cnst Oct 21 '15 at 4:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.