Re: StringNg review: MemBlob

On 09/22/2010 12:47 AM, Amos Jeffries 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);

The situation here is different, I believe, but I do not understand your
example. What is "a"? MemBlob does not have clear(). SBuf does not have
mem...

If s* are SBufs and you meant

   s1.clear(); s1.append(s1.buf(), s1.size())

then it is fine because s1.size() will be zero.

If you meant

   s1.clear(); s1.append(s2.buf(), s2.size())

then it is fine because s1.append will COW if s1 and s2 share MemBlob
and s2.size() is positive.

If you mean

  blob."clear"; blob.append(blob.mem, blob.size())

then it is fine because blob.size() will be zero.

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

The MemBlob append() code should not call memcpy() with a NULL
destination pointer, of course, but that is not related to the \note. My
understanding is that blob.mem is never NULL.

The MemBlob append() code must check for overflows. If I did not comment
on that before, please consider this a "please fix" comment.

Kinkie, while you are at it, please also avoid calling memcpy/move()
when size-to-append is zero, just in case.

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

Per \note, I think we are fine with memcpy() here but counter-examples
are more than welcome. I assume memcpy() is significantly faster in some
environments we care about.

On the other hand, if memmove() implementations we care about always
quickly check for overlaps and actually call memcpy() when possible,
then we should just use memmove()!

Thank you,

Alex.
Received on Wed Sep 22 2010 - 17:57:42 MDT