Comment by asveikau

2 days ago

A bit of a code review (some details from the patch removed for clarity):

   +       register int i;
           q = password;
   -       while((*q = getchar()) != '\n')
   +       i = 0;
   +       while((*q = getchar()) != '\n') {
   +               if (++i >= sizeof(password))
   +                       goto error;

You don't actually need i here. i is the same as (q - password). It would be idiomatic C to simply rewrite the loop condition as: while (q < password+sizeof(password) && (*q = getchar()) != '\n'). To preserve your "goto error;" part, maybe you could do the overflow check when null terminating outside the loop.

4 comments

asveikau

Reply
VonTum  9 hours ago

The article specifically mentions this optimization as not working with the compiler at that time, hence the need for the separate index variable.

> We will edit su.c to prevent the overflow by maintaining a counter, i, and verifying it against the buffer size during the read loop. I initially attempted a fix using pointer arithmetic, but the 1973 C compiler didn’t like it, while it didn’t refuse the syntax, the code had no effect. I settled on a simpler index-based check instead.

  • asveikau  3 hours ago

    Your sibling comment mentions it.

    There's no details.. why didn't it work? Maybe the author didn't write it correctly? No word on what the assembly output for it was...

shakna  2 days ago

Isn't sizeof only standardised in C89? Wouldn't shock me if this form needs to be an rvalue.

The author did try pointer arithmetic:

> I initially attempted a fix using pointer arithmetic, but the 1973 C compiler didn’t like it, while it didn’t refuse the syntax, the code had no effect.

  • asveikau  2 days ago

    This surprised me too. The snippet I was quoting from was already using sizeof, though.

    I missed the blurb about pointer arithmetic. Would be interesting to go into detail about what "had no effect" means.