[reSIProcate] Bug in Data::append()

[Byron Campen]

	I don't think mCapacity includes that last byte of room either. I  
think that last byte is reserved specifically for that null, and  
strictly off-limits for everything else. (Take a look at every "new"  
in Data.cxx, they all have that extra byte of room)

Best regards,
Byron

> Actually it looks like there is a problem if mCapacity == mSize + len
> and mMime != Share.  In this case by then end of the function  
> mCapacity
> == mSize and mBuf[mSize] is out of range.
>
> Perhaps the initial if statement should read:
> if (mCapacity <= mSize + len)
>
> Scott
>
> -----Original Message-----
> From: resiprocate-devel-bounces at list.sipfoundry.org
> [mailto:resiprocate-devel-bounces at list.sipfoundry.org] On Behalf Of
> Byron Campen
> Sent: Wednesday, March 22, 2006 5:45 PM
> To: Brian Cuthie
> Cc: resiprocate-devel at list.sipfoundry.org
> 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 at list.sipfoundry.org
>> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20060322/71b3970e/attachment.bin>