Re: StringNg review: MemBlob

On Wed, Sep 22, 2010 at 8:47 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 22/09/10 16:31, Alex Rousskov wrote:
>>
>> On 09/20/2010 08:18 PM, Alex Rousskov wrote:
>>>
>>> On 09/03/2010 09:21 AM, Kinkie wrote:
>>>
>>>> You'll find the branch at lp:~kinkie/squid/stringng
>>
>> ...
>>>
>>> Comments for MemBlob.cci r9472:
>>
>> Found one more:
>>
>>> * \note memcpy is used as the copying function. This is safe,
>>> * because it is guarranteed by design that the copied-to
>>> * memory area is only owned by the appended-to string,
>>> * and thus doesn't overlap with the appended-from area
>>> */
>>> void
>>> MemBlob::append(const char *S, const MemBlob::size_type Ssize)
>>> {
>>> memcpy(mem+bufUsed,S,Ssize);
>>> bufUsed+=Ssize;
>>> ++stats.append;
>>> }
>>
>> The \note is correct,
>
> No. As you pointed out to me earlier with SBuf the S here may be the result
> of:
>  a.clear(); a.append(a.mem, 1);

Using SBuf's raw memory access functions is dangerous, and this is
well documented.
It gives users rope, can't be blamed for how they use it..

>
> ...
>
>
>> but we should not mention "string" in MemBlob.cci.
>> MemBlob does not (or should not) know what its users are. There may be a
>> better way to express the same thought. Consider:
>>
>> \none memcpy() is safe because we copy to an unused area
>
> *that* is better. There is still no guarantee it wont overflow on the
> destination. memcpy() makes no mention about when happens when one of the
> pointers is NULL, but experience shows a lot of segfaults.

Changed to reflect this.
MemBlob.mem can never be NULL; we could add a guard if the user passes
a NULL char*, and treat it as a NOP.

> memcpy() docs state "always copies exactly num bytes" and that the buffers
> should not overlap.
> memmove() is the safe one which should be used if there is any doubt about
> overlapping memory. But even that can't validate against overflows.

Can change to memmove; is there a performance hit?

-- 
    /kinkie