#63 fix dbuf_c_str()

closed-fixed
Borut Ražem
None
5
2006-05-23
2006-05-15
Stas Sergeev
No

Hi.

There is a small bug, which is that dbuf_c_str()
calls dbuf_expand() for no aparent reason. The
problem is that it checks the part of an uninitialized
buffer, which has a random value. The bug is harmless,
but nevertheless it is a bug. The solution is to
initialize the buffer with zeros.
Patch attached.

Discussion

  • Stas Sergeev
    Stas Sergeev
    2006-05-15

    fix

     
    Attachments
  • Borut Ražem
    Borut Ražem
    2006-05-16

    Logged In: YES
    user_id=568035

    Hi Stas,

    dbuf_c_str() returns a null terminated string (buffer). It
    first checks if the buffer is already null terminated. If it
    is not, the buffer is expanded by 1 byte and null terminated.

    The problem is not that the uninitialized byte of the buffer
    is read, but that the fetched byte might be outside the
    allocated buffer if dbuf->len == dbuf->alloc.

    The proper fix would be:

    const char *dbuf_c_str(struct dbuf_s *dbuf)
    {
    assert(dbuf != NULL);
    assert(dbuf->alloc != 0);
    assert(dbuf->buf != NULL);

    /* only if not already null terminated */
    if (dbuf->len == dbuf->alloc ||
    ((char *)dbuf->buf)[dbuf->len] != '\0') {
    dbuf_expand(dbuf, 1);
    ((char *)dbuf->buf)[dbuf->len] = '\0';
    }

    return dbuf->buf;
    }

    Do you agree or I have overlooked something?

    Borut

     
  • Stas Sergeev
    Stas Sergeev
    2006-05-17

    Logged In: YES
    user_id=501371

    Hi.

    I think you are right that the real realloc() will happen
    only in that case. And perhaps you are right that the real
    bug (which I overlooked) is to access the char past the
    allocated space. But in case of len<alloc, dbuf_expand()
    will still be randomly called, depending on the content
    of an uninitialized char. Even though the realloc() won't
    take place (dbuf_expand() simply returns), I think randomly
    called function is still a bug.
    So I think it won't hurt if the both patches are applied.

     
  • Borut Ražem
    Borut Ražem
    2006-05-17

    Logged In: YES
    user_id=568035

    Fixed in release #4176

    Borut

     
  • Borut Ražem
    Borut Ražem
    2006-05-17

    • assigned_to: nobody --> borutr
    • status: open --> closed-fixed
     
  • Stas Sergeev
    Stas Sergeev
    2006-05-22

    • status: closed-fixed --> closed-wont-fix
     
  • Stas Sergeev
    Stas Sergeev
    2006-05-22

    Logged In: YES
    user_id=501371

    No, the valgrind still thinks that code is buggy (as it
    actually is).

     
  • Borut Ražem
    Borut Ražem
    2006-05-22

    Logged In: YES
    user_id=568035

    Fixed (hpoefully) in release #4184.

    Stas,

    I hope now both you and valgrind will be satisfied, even
    that I don't fully agree that the read of uninitialized but
    properly allocated memory is a bug. I agree that it is a
    potential bug, which should be reviewed. So I wold call it a
    "gray zone".

    Now dbuf_expand() is called and the null character is
    appended at the end of the buffer regardless if it was
    already there.

    Can you please tell me what message valgrind showed? Was it
    a warning or an error?

    In addition I fixed a bug where the buffer was enlarged even
    if it was large enough.

    Thanks,
    Borut

     
  • Stas Sergeev
    Stas Sergeev
    2006-05-23

    Logged In: YES
    user_id=501371

    Thanks, I'll test the fix today.

    Yes, I agree it is a potential bug only, yet why not
    fixing it if it is so simple? :)
    I think valgrind meant an error, but I am not sure.
    He probably can't really distinguish - this *is* an
    error after all, however, because of an extra checks in
    dbuf_expand(), it is neutralized at a later stage.
    Attached is the valgrind log. Ignore the final free()
    bug and SIGSEGV - I already opened a separate report
    for that (not yet fixed). The first messages about the
    uninitialized values are from dbuf.c

     
  • Stas Sergeev
    Stas Sergeev
    2006-05-23

    valgrind log

     
    Attachments
  • Jim Paris
    Jim Paris
    2006-05-23

    Logged In: YES
    user_id=175928

    > I don't fully agree that the read of uninitialized
    > but properly allocated memory is a bug.

    FYI: On ia64 doing anything with uninitialized memory
    can be dangerous due to the NaT bit: see e.g.
    http://blogs.msdn.com/oldnewthing/archive/2004/01/19/60162.aspx

     
  • Stas Sergeev
    Stas Sergeev
    2006-05-23

    • status: closed-wont-fix --> closed-fixed
     
  • Stas Sergeev
    Stas Sergeev
    2006-05-23

    Logged In: YES
    user_id=501371

    OK, this fix the valgrind doesn't mind.

     
  • Borut Ražem
    Borut Ražem
    2006-05-23

    Logged In: YES
    user_id=568035

    Jim,

    thank you to enlighten me: I didn't know about the NaT bit.

    I'm not sure if the example from the article applies to our
    case, but anyway: the bug is fixed and everybody is happy
    now ;-)

    Borut