Fixing a 37-year-old bug by merging a 22-year-old fix | Hacker News Reader

Fixing a 37-year-old bug by merging a 22-year-old fix

cvsweb.openbsd.org

268 pointstscherno72 comments

This fix leaves a variable uninitialized, and depends on funny logic to later give it a value.

The general structure should be

  FILE *fp = stdin; /* sane default */

then override with a new pointer from fopen if necessary.

In fact, looking at this more closely, note how fp is scoped to the body of the for loop. That's where it should be defined:

   for (firsttime = 1; ; firsttime = 0) {
     FILE *fp = stdin;

     /* logic flips it to fp = fopen(...) as necessary */

   }

[Note: the C compiler Joy was working with probably didn't support block scope declarations except at the top of the function. This comment applies to merging the fix using the environment of today.]

Note how the fclose at the bottom of the loop ends up closing stdin in the case that fp == stdin! It ensures that this only happens once, but you have to analyze it to see that this is true. I would write this as:

     if (fp != stdin) /* defensive! */
       fclose(fp);

Making it an infinite loop terminated by calling exit is unnecessary. There is already a continue in the loop, and a break in an inner loop; another break instead of the exit, and a "return status;" at the bottom wouldn't hurt. Better yet:

  /* Enter loop whenever there are args left to
     process, or even if there are no args if it is
     the first time through (to process stdin instead). */

  for (firsttime = 1; firsttime || *argv; firsttime = 0) {
    FILE *fp = stdin;

    if (*argv) {
       /* have args; do the fopen to replace stdin */
       /* set status = 1 and continue if cannot fopen */
    }

    /* do head logic */

    if (fp != stdin)
      fclose(fp);
  }

  return status;

C itself was only a couple of years old when that code was written originally, and the systems were small. Whereas I've been using it since 1989. Not criticizing the original!

As I understand it, you can fclose(stdin) without issue on almost all POSIX systems.

Yes, I know this, and in a throwaway little program where you are sure you are the last one to ever touch stdin (because you're calling exit right there in the loop, for instance) it is okay.

In old sysvinit-style daemons, closing stdin/stdout/stderr (or redirecting them to /dev/null) was a necessary part of the daemon initialization process.

Sure, but it's surprising, and in code, 'surprising' usually translates to 'wrong'.

Not sure that scoping to the block + unconditionally doing the initialization actually helps readability, though I do think doing one or the other would.

Also the original is super efficient which is pretty cool, and (at least when correct) is arguably more readable (probably not so much re. extendable) since it seems like there's no way to look at the code and think you know what it's doing when you actually do not.

I would argue your insistence on giving the variable a "sane default" is a matter of opinion. I personally am much happier seeing a variable left uninitialized than ever seeing it double-assigned, as to me a fundamental semantic logic error has occurred if you are overriding the value of this kind of variable, especially as part of a conditional later where it might not be clear whether or not it happened: I really would prefer to look at the variable being assigned and think "ok, now we know the value". The compiler is entirely able to determine whether or not the variable has been initialized in all possible control flows for a case like this (at least, once we move the scope of the variable to inside of the for loop, of course), so there is no safety benefit to hoisting that default assignment out, nor is it saving a meaningful amount of code: it only serves to obfuscate the "default" control flow by making what had been an explicit if/else into an implicit if/default :/.

Always initialize your variable. There is not aesthetic argument, but there are multiple software engineering argument against failing to initialize variables.

For starter, you don't have prescience. You can't prevent future version from growing new alternate code paths. As the code evolve, checking that every initialization-free variable will still get initialized is a burden and easily overlooked. Then there are more potent evolution that will make it just impossible: the code will get generalized, or moved into a library, or get a a context object to be able to run multiple instances. Or will get a state machine to handle complex combination of options.

You're relying on the compiler telling you the variable will get initialized. But the compiler may change. Or the human compiling it might not look at warnings.

OTOH, double-iniitalization is mostly a myth. If the compiler can tell the variable may not get any initialization and warn you are exactly the same as the case the optimizer will be able to see the double init and optimize it out. I've looked at generated code and see compilers properly eliminate redundant stores.

You're better off betting on optimization and get bitten by occasional double-stores than get bitten by uninitialized variables and give system penetrators yet another free lunch.

If you use static analysis, defensively initializing to a wrong value may mean that your tooling doesn't tell you when you fail to correctly set it from that wrong value.

I have seen this argument numerous times before.

It doesn't hold water, because potentially initializing variables to wrong values is just as much as consequence of using that static tool, as it is a consequence of an "always initialize" coding habit.

The static tool produces a report of uninitialized variables or potentially uninitialized ones. The developers react to this by fixing all those instances, so that the tool is "shut up". Once that happens, they have a code base for which the tool doesn't report anything, just like the other developers who always initialize.

There is no reason to believe that either team has a lower or higher rate of incorrect-initial-value bugs. Or is there?

I would argue that the developer who is writing the code, at that moment, quite possibly has a better idea of what the correct initial value should be, than someone who is running a tool across a large code base and following up (by him or herself).

Really, the coding convention should be "always initialize, with the correct value which doesn't need to be overwritten later."

Even in imperative languages, we should program as "functionally" as is reasonably possible.

Imperative code that mutates local variables should only do so if it implements iteration. If the code has no loops and is setting local variables conditionally, that shows that it's trying to influence the behavior of some common code where the control flow paths merge --- instead of putting it into a function!

In the specific program we are looking at, for instance, the function is trying to use the head processing block at the end, but over different input streams. This can be done by putting it into a function, so then there is no assignment to the fp variable:

   if (!*argv) {
     head_processing(stdin);
   } else {
     FILE *fp = fopen(*argv, "r");
     /* check for fopen failure ... */
     head_processing(fp);
     fclose(fp);
   }

We can eliminate assignments out of loops, too, but that requires tail recursion:

   /* pseudo code */
   head(int argc, char **argv, int first_time)
   {
     if (argc == 0 && first_time) {
       return head_processing(stdin);
     } else if (argc == 0) {
       return 0;
     } else {
       FILE *fp = fopen(*argv, "r");
       /* head_processing checks for null */
       int status = head_processing(fp);
       fclose(fp);
       
       /* tail call, if status was good */
       return (status != 0) 
              ? status : head(argc - 1, argv + 1, 0); 
     }
   }

C programmers will tend not to be comfortable with this style, and it requires tail call support in the compiler to not generate O(N) stack. So for pragmatic reasons, you can't make a coding convention out of this.

No tool will help you much if you don't know how to use it. In this case, using the tool wrong, one failure mode puts you in the same situation as not using the tool and defensively initializing. That is a very poor argument that the tool does no good.

That argument isn't that the tool does no good. The tool is useful even if you follow the practice of always initializing: it helps catch accidental deviations from that practice.

Well sure. My point is that it basically devolves into checking that if you treat it poorly, and can be more useful if you don't. If you make a habit of initializing variables with the wrong value, it's less useful.

My other comment was in response to the first half of your comment, before an edit added more about structure of code. I'd say I partly agree, but I think another consideration is that in C there is a limit to how much you can reasonably fit in an expression. What in, say, Haskell might look like...

    let f = case something of
                Foo -> 10
                Bar -> 22

cannot reasonably take the same form in C, despite there not being a "real" update going on.

This means in C you wind up writing things like,

    int f;

    switch(something) {
        case Foo:
            f = 10;
            break;
        case Bar:
            f = 22;
            break;
        default:
            /* ... handle error ... */
    }

And the flow is basically the same, but with an additional degree of freedom to make a mistake, and there some static checking can have your back.

Incidentally, on GCC and Clang I recommend building with -Wswitch-enum, which will help point you at the above when you add something to the enum that contains Foo and Bar.

The compiler will issue a warning if a code path uses an uninitialized value.

My issue is not performance: I believe that you will have more coding errors due to the semantic error inherent in the variable being initialized twice. In some languages... in many languages... this is in fact impossible to even type, and the arguments why are due to semantics, not performance.

If it makes you feel better, move this code to a function that returns a single value and initialize the variable once in one place (and then mark e value const!). I actually also agree this is a better option, but semantically the if/else is clearer than the if/default, so I would not fall back to if/default over if/else.

As for relying on the compiler: turn that warning on manually and turn that warning into an error manually. (Do not turn all warnings on, as upgrading your compiler can break your build, but turn on warnings that you actively want to verify your code against and have cleared your code against (including this one.)

I will also argue that you should treat your static analysis similar to a dynamic unit test on your code: it doesn't matter if some people are using a compiler that doesn't have this feature (though that seems unlikely as I believe all major compilers support this feature) as the maintainer will, in the same sense that running unit tests sometimes requires more dependencies than running the underlying code.

Again, though, if you are very used to the if/default control flow, I can appreciate wanting to use it: I just would rather see people take advantage of static language analysis that is present to deal with silly errors, and then arrange their code to better handle high-level logic errors.

The extremely odd for-loop structure feels to me like someone was trying very hard to avoid a goto, and in the process obfuscated the flow quite a bit more. The special case of head'ing stdin is similar to the classic "loop and a half problem" (which is really only a problem if you religiously abstain from goto use.) I'd consider this a more straightforward way to do it:

    FILE *f;
    int i = 1;
    ...
    if(argc < 2) {
       f = stdin;
       goto do_head;
    }
    ...
    while(i<argc) {
       f = fopen(argv[i], "r");
       /* check for failure to open */
       ...
      do_head:
       /* head'ing code goes here */
       ...
       fclose(f);
       i++;
    }

That being said, OpenBSD code is still far more concise and readable than GNU; compare coreutils' head.c here:

http://code.metager.de/source/xref/gnu/coreutils/src/head.c

GNU head has a bit more functionality, but I don't think the increase in complexity - nearly 10x more lines - is proportional to that.

Here's the version used in OS X. It's a bit bigger but not GNUishly big.

http://www.opensource.apple.com/source/text_cmds/text_cmds-8...

That's almost certainly the FreeBSD one.

Looks like a small extension to the BSD one, that can also handle head'ing by byte count.

(Although I think the use of malloc() and strcpy() to handle the -[0-9]+ case seems rather unnecessary...)

In a 24-year-old version control system.

edit: Wow, not sure what's offensive here...

You're probably getting downvoted because a lot of OpenBSD threads derail into fruitless discussions of their use of CVS.

People here downvote what they don't like

I'm well aware of that, the question is why don't they? I found it a neat aspect that an long-lived bug with a long-lived PR is in a long-lived VCS.

You're single line comment might have appeared dismissive of the version control used by the OpenBSD people, especially because it's something that often comes up in threads about BSD.

Threads involving OpenBSD can involve fierce downvoting so it's probably a good idea to be clear about what you're saying.

I think all of this could be easily avoided if HN stopped allowing people to try and silence valid comments that they happen to disagree with.

The site is now more political than ever and this just seems to fuel the "silence what you don't agree with" type voting, which I don't think is a good thing.

Topic aside, the original comment was ambiguous. Is it a middlebrow dismissal ("Pfft, their VCS is two dozen years old ..."), or is it an expression of admiration ("Wow, it's pretty amazing that their version control system has stood the test of time, even when the rest of the world seems to be moving towards git!")?

The pythonic tendency towards being explicit about what we mean to say serves us well even in prose. :-) I agree with the OP, it's a pretty neat engineering feat. I think the downvotes would likely have been avoided had that admiration been more clearly stated.

or what they consider irrelevant or noise.

That's only true in the first hour or two or a comment's existence. If you wait until most active HNers have had a chance to weigh in, it's almost always only the content-free, Reddit-style (eg., "that's true!") or blatantly offensive comments that are downvoted below 0.

I posted this in the reddit thread today, after being part of a larger discussion last week.

Theo de Raadt co-created AnonCVS back in 1995/1996. He and Chuck Cranor from AT&T Labs Research published a paper in 1999 about it.

OpenBSD was the first open source project to make access to their source repository public to non-developers, before AnonCVS you needed commit access to review history, annotations and even checkout. OpenBSD had a read-only CVS tree even before their website was created in 1996.

There are no known prior cited examples of a project providing this level of transparency to the development process, most open source projects at the time only provided "snapshots" of development in the form of compressed source directory tarballs, excluding RCS files. SUP and FreeBSD's ctm(1) only let you merge "current" remote changes to a local non-versioned directory.

http://www.openbsd.org/papers/anoncvs-paper.pdf

http://www.openbsd.org/papers/anoncvs-slides.pdf

And here's an additional historical tidbit relating to FreeBSD and AnonCVS: https://twitter.com/canadianbryan/status/517714611089719296

This link to the commit log is slightly better:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/he...

You're one click away from the diff.

What's with the previous commit about a "DDOS" - it's just changing exit(0) into exit(status)?

There must be more context to that. CVS doesn't have change sets; just from looking at this log, we don't know what else may be related to this.

Maybe some important shell scripts hang in a loop because they expect head to report an unsuccessful termination status when a file doesn't exist. Maybe some situation exists where a privileged system script can be fooled into looping by an unprivileged user.

Here is the mailing list discussion:

https://www.mail-archive.com/misc@openbsd.org/msg132628.html

There is no report of any actual DDoS. Craig Skinner was really just testing tools on nonexistent files!

So the commit comment just follows from some general hypothesis that incorrect exit statuses from tools can be exploited in some way by attackers.

Looks like that's it, and the commit log was probably just sarcastic then. :)

Diff: http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/he...

One of my coworkers found a data integrity causing bug that has been around for 11 years.

He and his team lead had no idea how such a bug missed 11 years of peer-QA through all edits on that module / activity.

It was as simple as converting to string and back and hitting an edge case on internationalization with commas vs periods.

Those are the ones which scare me the most.. when you find them, they're potentially devastating, and you haven't hit them.

Another I've seen is the potential landmine that can go of at ANY time and then one day, 5 years in the future, it hits you at 2AM and you can't figure out WTF is happening.

My favourite decimal separater i18n bug was in a product where we were reading a time dilation factor from a config file. In the default shipping file (which basically nobody ever changed) it was specified as "1.0". We would then read this using (this is in .Net) Double.parse() which honors the users locale if none is specified, so for all users in Europe, and probably elsewhere, this was interpreted as 10 ('.' is a grouping construct and more or less ignored). So the software was running 10x slower than expected.

I shipped a program with the exact save bug! And I've worked on at least one website with the same bug. ASP.NET uses HTTP headers to determine locale and will parse/format on it. Which means if someone used the currency specifier for some reason, all of a sudden they're displaying the wrong prices!

Or, if the website reads some string from the back end, like say a config value "DefaultCreditLimit", the user night be able to modify the interpretation of it.

I've come to the conclusion that this automatic behavior that tries to guess at the format required is simply wrong. If a program wants user-specific formatting, it should opt-in.

. would be a weird here as well (and we had a share of issues with early online banking systems: Think "I want to pay 10 $currency, let's put 10.00 - and the bank uses the . as a delimiter for thousands, i.e. 1.000,00 / you meant 10,00).

That said, my favorite localization issues stem from CH/Switzerland. Numbers are formatted as 1'000.00 there (someone correct me), which is especially odd:

1) That's a weird decimal separator. DE uses a , and as far as I know FR uses a , as well. I _assume_ IT uses a , - so why is this a . in CH?

2) The ' really kills a lot of naive parsers and causes quite a bit of frustration in other fields (think OCR - some overly fat 1'000 might turn into a 11000 with bad luck/crappy image quality or (marginally better) lead to 1000 where means 'I think here's a character, but no clue what that might be')

Different currency has different conventions?

I'm confused.

Yes, different conventions exist (in this case the thread drifted to bugs based on locales, so obviously we're talking about different conventions here). I .. don't understand what you're saying.

(and my original post is missing an asterisk in two places, as in 1?000 as OCR result for example - messed up the formatting in the middle of the night)

Sorry - my point was counter to your about DE, FR and IT having the same conventions, so it's odd CH doesn't too. They all use the same currency, whereas CH doesn't, hence why there'd be a different one (?). E.g. we use £1,000.00 in the UK.

DE, FR and IT have the same currency only the last 15 years or so. One would expect that they would be using the same conventions because of the history and relationships between CH and all of these countries.

1) This is actually not that clear cut. Both , and . are used and it depends on context which is used where (usually within the same business sector the guidelines are consistent).

2) Actually, I find it much more confusing that people are constantly using . and , for two completely different functions and not get confused about it. Mistaking a . for a , and vice-versa is quite easy.

And that not even considers the likely confusion for people coming from countries with different rules.

OTOH an ' is highly distinct visually from both a , and . so it's always clear that it can't be the decimal separator.

Satellite control software: you enter tiny time intervals using scientific notation to command very short thruster bursts for realignment. User A, the usual guy, always uses lowercase 'e', no problem. One day he goes on holiday, user B takes over, and on his first day enters a value using uppercase 'E'. The s/w is not expecting this, and so interprets 7.6E-3 seconds as 7.6 seconds (e.g., exact numbers unknown).

Fortunately, after the predicable mass panic, they were eventually able to relocate the satellite.

> The s/w is not expecting this, and so interprets 7.6E-3 seconds as 7.6 seconds

This sounds extremely sloppy, especially for satellite control software. Anything not fitting exactly the expected format should lead to an error so the operator can immediately correct it, not be silently ignored.

Are you running FxCop? Specifically CA1305: Specify IFormatProvider (http://msdn.microsoft.com/en-us/library/ms182190.aspx) should help with this class of bugs - or at least make the bug easier for reviewers to catch (CultureInfo.CurrentCulture stands out in config parsing code).

Right now I'm working on our company's new commissions payment system. Thanks to you I'm going to have nightmares tonight.

I like that the -number option is "obsolete". I still use it.

That's pretty funny. I'd say that the majority of the time that I call head or tail, it's using that syntax.

Exactly, and I bet that it's not going away any time soon, "obsolete" or not.

Here is the diff:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/head/he...

If OpenBSD has anyone young enough, would've been a fun twist to have someone who wasn't even born at the time of the fix make the commit.

> for (firsttime = 1; ; firsttime = 0) {

does this line predate the while loop?

or boolean values in C?

while loop right above the for loop:

> while ((ch = getopt(argc, argv, "n:")) != -1) {

bool was introduced in C99

C99 introduced the boolean stdlib, so yes it predates that.

Also, as the other guy said, that code has while loops.

What's the common way of doing this in C? Is it normally something like this?:

  firsttime=True
  while (True) {
  ..
  ..
  ..
  firsttime=False
  }

That's probably the cleaner way to do it, but honestly, the hacked for-loop feels pretty clean too. And it ensures that your firsttime flag is properly set, regardless of how you break out of the loop (a 'continue' in the middle of your example would fail to set the flag properly).

Depends on the person. There likely is a correlation on what century you learned programming, but certainly one whether you have experience with pascal-ish languages.

People educated in the Wirth way would say "that's not a for loop; there is no way to tell how often it will iterate when it is first entered" and write it as a while loop.

Old-school C programmers seem to think one iteration construct is enough for everybody and like to write any iteration as a for loop, ideally with a few commas and no actual statements, as in the classic strcpy:

  char *strcpy(char *dst, CONST char *src)
  {
    char *save = dst; 
    for (; (*dst = *src) != '\0'; ++src, ++dst);
    return save;
  }

(from http://www.ethernut.de/api/strcpy_8c_source.html)

Programmers with soe Wirth-style training do at least something like

  char *
  strcpy(char *s1, const char *s2)
  {
    char *s = s1;
    while ((*s++ = *s2++) != 0)
	;
    return (s1);
  }

(from http://www.opensource.apple.com/source/Libc/Libc-167/gen.sub...)

[If you google, you can find way more variations on this. for example, http://fossies.org/dox/glibc-2.20/string_2strcpy_8c_source.h... does a do {...} while(....) that I am not even sure is legal C; it uses offsets into the source to write into the destination]

From your example I guess you are definitely in the Wirth (Pascalish) camp, beyond even that second state. Classic C programmers would either use 0 and 1 for booleans or #define macros FALSE and TRUE (modern C has true booleans, but that extra #include to get it increases compilation time, so why do it?

I am in te Pascal-ish camp. I like to say C doesn't have a for loop because its 'for' is just syntactic sugar for a while loop.

I find the for loop easier to read.

It's not a while loop... well, maybe a "while 1 {}" loop.

It's closer to an iterator over argv.

First, it sets the firsttime=1 flag, then it tries to open the first argument (file). After the first arg, it sets firsttime=0. It only breaks out of the loop once argv is exhausted.

>does this line predate the while loop?

It's the other way around. Very early C didn't have a for loop. It seems it was introduced in Unix V5: http://minnie.tuhs.org/cgi-bin/utree.pl?file=V5/usr/c/c00.c

And a for loop is basically just a while one, but they are sometimes used in different ways than the usual `for (size_t i = 0; i < len; ++i)`. One of my favorites is http://git.musl-libc.org/cgit/musl/tree/src/internal/floatsc....

>or boolean values in C?

1 and 0 are boolean values in C. But C didn't have a boolean type until C99 (called _Bool or bool after including <stdbool.h>).

What does that loop even do?

The whole function is trying to parse a float value, that loop is looking for the string "Infinity".

c|32 will do a bitwise downcase of an ascii character so, "INFINITY", "infinity", "INfinITy" will all be accepted. In any of those cases i == 8 at the end of the loop and is still in scope, so the following if statement will ultimately return + or - Infinity.

ASCII case-insensitive comparison of up to 8 characters from a stream with "infinity".

  > What does that loop even do?

  for (i=0; i<8 && (c|32)=="infinity"[i]; i++)
    if (i<7) c = shgetc(f);

It's a pattern matching loop. The 'c' variable is a character which is read from FILE * f. In this case, it's attempting to match the string "infinity" which has a length of 8. With ASCII, uppercase letters start from hex value 0x41 for the letter 'A'. Lowercase letters start from hex value 0x61 for the letter 'a'. Thus the difference between lowercase and uppercase is 0x20 which is 32 decimal or 100000 in binary. Thus, if you or the value of 'c' with 32 (0x20 or 00010000), you're setting the 2^5 bit of the character which converts uppercase to lowercase and leaves lowercase unchanged since all lowercase characters already have that bit set.

I'm sure you can figure out the rest. As long as the character matches the corresponding index position in the character array "infinity", the for loop keeps matching characters. Eventually, a successful match results in a return value of sign * INFINITY from the function.

Woe to the developer trying to parse 0x13a0ce!

There are no booleans in C, just ints. I think c99 added a 'bool' type, but it's just typedef'd to being an int.

That is not correct. In C99, _Bool (and the typedef bool) are special, because when any value is converted to _Bool, the result is 0 or 1 (!!x). Although the standard does not explicitly say that _Bool can only hold anything other than 0 and 1 (only that it is capable of representing these values), this seems to be the case, effectively.

It's a bit more, the bool type can only hold two values, 0 and 1. (6.3.1.2 Boolean type #1) "When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1."

Looking through all these comments, my only response is "effing nerds..." <3

Reply on news.ycombinator.com