Re: readstring() bug

[Markus Mauhart]

"Paul D. Smith" <address@hidden> wrote in message
news:address@hidden
> %% Markus Mauhart <address@hidden> writes:
>
>   mm> function readstring() in file read.c has 3 bugs, two of them
>   mm> concerning backslash newline detection are so killing that IMHO
>   mm> either complete readstring() or at least its backslash newline
>   mm> detection functionality must be dead code ... is this conclusion
>   mm> correct ? (sources from v381beta1)
>
> Of course not.  You can easily determine for yourself that the code is
> not dead.
>
>
> What kinds of problems do you see?


maybe it's too late today for me, but for me it seems to be very obvious:

original is ...

////////////////////////////////////////////////
static unsigned long
readstring (struct ebuffer *ebuf)
{
  char *p;

  /* If there is nothing left in this buffer, return 0.  */
  if (ebuf->bufnext > ebuf->bufstart + ebuf->size)
    return -1;

  /* Set up a new starting point for the buffer, and find the end of the
     next logical line (taking into account backslash/newline pairs).  */

  p = ebuf->buffer = ebuf->bufnext;

  while (1)
    {
      int backslash = 0;

      /* Find the next newline.  Keep track of backslashes as we look.  */
      for (; *p != '\n' && *p != '\0'; ++p)
        if (*p == '\\')
          backslash = !backslash;

      /* If we got to the end of the string or a newline with no backslash,
         we're done. */
      if (*p == '\0' || !backslash)
        break;
    }

  /* Overwrite the newline char.  */
  *p = '\0';
  ebuf->bufnext = p+1;

  return 0;
}
////////////////////////////////////////////////


what I see is ...

1) first the minor buggy

  if (ebuf->bufnext > ebuf->bufstart + ebuf->size)
     return -1;

... if ebuf->size is really malloc'ed size, then this must be ...

  if (ebuf->bufnext >= ebuf->bufstart + ebuf->size)
     return -1;

... otherwise the later loop could read outside the allocated memory.

2)
the while loop, which AFAICS should iterate over all EOL-terminated
substrings until it finds an EOL that is not escaped with odd-numbered
backslash, counts all backslashes in that substrings, instead of only
the neighbours of EOL.

3)
regardless of the mentioned wrong BS EOL detection, the loop will never
really loop, cause after the 1st run it still sits at the 1st EOL, hence
the 2nd run will find zero BS before the old EOL and therefore break
without reading anything new.

So AFAICS the whole loop as it stands now is equivalent to ...

    {if (strchr(p,EOL)) p = strchr(p,EOL);}

And btw, the "return 0" looks a bit surprising too.


IMHO a correct version would be ...


////////////////////////////////////////////////
static unsigned long
readstring (struct ebuffer *ebuf)
{
  char *eol;

  /* If there is nothing left in this buffer, return 0.  */
  if (ebuf->bufnext >= ebuf->bufstart + ebuf->size)
    return -1;

  /* Set up a new starting point for the buffer, and find the end of the
     next logical line (taking into account backslash/newline pairs).  */
  eol = ebuf->buffer = ebuf->bufnext;

  while (1)
    {
      char* const begin = eol;
      char*       bs;

      eol = strchr (begin, '\n');
      if (eol == 0 || eol == begin || eol[-1] != '\\') break;

      /* Now let bs points to the leftmost BS of this EOL: */
      for (bs = eol - 1; bs != begin; ) if (*--bs != '\\') {++bs ;break ;}

      /* "eol - bs == n" <=> "have exactly n BS before this EOL" */
      if ((eol - bs) % 2u == 0)
          /* This EOL has no dedicated BS, hence the line is over: */
          break;
      else
          /* Move on to the next char in this line: */
          ++eol;
    }

  /* Overwrite the newline char.  */
  *eol = '\0';
  ebuf->bufnext = eol + 1;

  return 0;/* really 0 ? */
}
////////////////////////////////////////////////


Regards,
Markus.