[sdcc-devel] [ sdcc-Patches-1489008 ] fix dbuf_c_str()

[SourceForge.net <no...@so...>]

Patches item #1489008, was opened at 2006-05-15 21:38
Message generated for change (Comment added) made by stsp
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=1489008&group_id=599

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Closed
Resolution: Wont Fix
Priority: 5
Submitted By: Stas Sergeev (stsp)
Assigned to: Borut Razem (borutr)
Summary: fix dbuf_c_str()

Initial Comment:
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.

----------------------------------------------------------------------

>Comment By: Stas Sergeev (stsp)
Date: 2006-05-23 07:57

Message:
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

----------------------------------------------------------------------

Comment By: Borut Razem (borutr)
Date: 2006-05-22 23:50

Message:
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


----------------------------------------------------------------------

Comment By: Stas Sergeev (stsp)
Date: 2006-05-22 20:43

Message:
Logged In: YES 
user_id=501371

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

----------------------------------------------------------------------

Comment By: Borut Razem (borutr)
Date: 2006-05-17 20:25

Message:
Logged In: YES 
user_id=568035

Fixed in release #4176

Borut

----------------------------------------------------------------------

Comment By: Stas Sergeev (stsp)
Date: 2006-05-17 08:02

Message:
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.

----------------------------------------------------------------------

Comment By: Borut Razem (borutr)
Date: 2006-05-17 00:51

Message:
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

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=1489008&group_id=599