< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

RE: [reSIProcate] Bug in Data::append()


Byron,

Are you sure? Looks to me like the buffer is mSize in length. Here's the
relevant parts:

mBuf = new char[mSize + len];
mSize += len;
mBuf[mSize] = 0; // Buffer overflow

I'm using the 0.8.0 tarball.

-brian
 

> -----Original Message-----
> From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx] 
> Sent: Wednesday, March 22, 2006 5:45 PM
> To: Brian Cuthie
> Cc: resiprocate-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [reSIProcate] Bug in Data::append()
> 
>       Actually, that buffer has size mSize+1, so that null 
> termination is not causing memory corruption. But, your 
> heap-corruption issue is still real, so I ask what version of 
> resiprocate you are using.
> 
> Best regards,
> Byron Campen
> 
> 
> >
> > Hello,
> >
> > I'm new to this list, and to using the Reciprocate stack. 
> However, I 
> > think I found a bug and wanted to hear what people think. 
> I'm sure I'm 
> > doing this wrong, so please don't be shy about helping me 
> understand 
> > the correct procedure.
> >
> > Background:
> >
> > While testing the stack under Windows, I had problems with a heap 
> > corruption when writing debug log messages using the 
> VSDebugWindow log 
> > type.
> >
> > BUG:
> >
> > The following code in Data::append() appears to be in 
> error. The NULL 
> > is written past the end of the buffer, and furthermore I 
> think it is 
> > unnecessary, since c_str() adds a NULL.
> >
> > Data&
> > Data::append(const char* str, size_type len) {
> >    assert(str);
> >    if (mCapacity < mSize + len)
> >    {
> >       // .dlb. pad for future growth?
> >       resize(((mSize + len +16)*3)/2, true);
> >    }
> >    else
> >    {
> >       if (mMine == Share)
> >       {
> >          char *oldBuf = mBuf;
> >          mCapacity = mSize + len;
> >          mBuf = new char[mSize + len];
> >          memcpy(mBuf, oldBuf, mSize);
> >          mMine = Take;
> >       }
> >    }
> >
> >    // could conceivably overlap
> >    memmove(mBuf + mSize, str, len);
> >    mSize += len;
> >    mBuf[mSize] = 0;   // ERROR
> >
> >    return *this;
> > }
> >
> > Finally, there appears to be a related bug fix to Data::c_str() 
> > (#5408).
> > I'm wondering if someone was tracking this same problem and 
> tried to 
> > fix it there, since resize() will copy the buffer anyway, 
> making the 
> > check in c_str() unecessary.
> >
> > Cheers,
> >
> > Brian
> >
> > _______________________________________________
> > resiprocate-devel mailing list
> > resiprocate-devel@xxxxxxxxxxxxxxxxxxx
> > https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
> 
>