Re: still getting errors with CRM

On Thu, May 01, 2008 at 12:51:12AM +0200, Ger Hobbelt wrote:
> It's not about strings with '\' as the LAST char, it's about strings
> which have a '\' FOLLOWING them, i.e. a '\' which is NOT PART OF THE
> STRING.
> 
> Think
> buf[] = "hello".
> That's
> buflen=5
> so 'o' is at
> buf[4] (remember, 'C' is zero-based)
> and the '\' is here:
> buf[5] == '\\'   !!!

Being the incurably curious software engineer that I am,
this piqued my curiosity because the length of a 5-character
string is typically *six* characters to account for NULL
termination.  At the very least, an outside view may
help....

Looking at the code in the attached file, all loops on the
string look like this:

    for (is = 0; is <= inlen ; is++)

Because the loop is <= (and it *should* be that rather than
'<' which is what I would normally expect), this means that
'inlen' is 1 character less than the input length of the
string.

The actual code in the attached file has this at line #139:

  buf[inlen] = ' '; // prove bad behaviour: this is written outside buf valid range, so should have no effect

However, the comment is wrong.  Because of the loop
definition, you are changing data which is being processed.

Of course, the request to "set buf[inlen] = '\\'"
illustrates what looks to me like a *major* boundary-condition
bug assuming that the string in 'buf' is *not* necissarily
NULL-terminated (which I am guessing is the case as othewise
there should be a check to verify "buf[inlen] == '\0'").

The code starting at line #173 looks like this:

================================================================================
    for (is = 0; is <= inlen ; is++)  
    {
      if (buf[is] != '\\' )
      {
        buf [id] = buf [is];
        id++;
      }
      else
      {
        //  we're looking at a '\\'.
        //  
        //   Check for a few common things: \n, \a, \xNN, \oNNN
        is++;
        //
        switch (buf[is])
        {
================================================================================

(I verified this is unchanged from that available to me in
the debian source archive)

Note the line that increments the variable 'is' using
"is++;".  If the code is already at the position "is ==
inlen" (the last iteration in the loop), the switch
statement is now processing the location buf[inlen+1].

Now, perhaps something is going on here that I am missing,
but since there is a check to verify that 'inlen <= maxlen',
this implies the code is trying to process invalid data.

I'm guessing the code should look something like this:

================================================================================
    for (is = 0; is <= inlen ; is++)  
    {
      if (buf[is] != '\\' )
      {
        buf [id] = buf [is];
        id++;
      }
      // >>>>>> THIS IS NEW CODE <<<<<
      else if (buf[is] == '\\' && is == inlen)
      {
        // Special boundary condition to prevent the 'else'
        // block from reading past inlen.

        // Reaching this location means that a single
        // backslash character appears at the end of a
        // string repesenting the start of an escape
        // sequence (handled in the else block) with no
        // actual escaped content (can't escape the end of
        // the string!).  This assumes, BTW, that 'buf' does
        // not start NULL terminated.

        // No clue what should actually be done here....
      }
      // >>>>>> END THE NEW CODE <<<<<
      else
      {
        //  we're looking at a '\\'.
        //  
        //   Check for a few common things: \n, \a, \xNN, \oNNN
        is++;
        //
        switch (buf[is])
        {
================================================================================


As I said, I could be completely off-base and spewing
irrelevant garbage (wouldn't be the first time!), but
thought I'd provide some input as it sounds like this has
become a frustrating issue for many people.

My guess, however, is that a single backslash at the end of
strings is what is causing the intermittent error.  That, of
course, is probably being caused by something else in the
code (which I could believe is a 64-bit issue).

The alternative to this, of course, is that buf[inlen]
is expected to be a null-termination character (making the
above added if-else un-necessary), which in a routine like
this would be good to check for to prevent the noted buffer
overflow on "is++;".

--David


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone