Jeff King <peff@xxxxxxxx> writes:
>> > const int blk = 1024;
>> > long trimmed = 0, recovered = 0;
>> > - char *ap = a->ptr + a->size;
>> > - char *bp = b->ptr + b->size;
>> > + char *ap = a->ptr ? a->ptr + a->size : a->ptr;
>> > + char *bp = b->ptr ? b->ptr + b->size : b->ptr;
>> > long smaller = (a->size < b->size) ? a->size : b->size;
>> >
>> > while (blk + trimmed <= smaller && !memcmp(ap - blk, bp - blk, blk)) {
>>
>> Isn't it a bug for a->ptr or b->ptr to be NULL here? Even if we
>> manage to assign ap = a->ptr = NULL without complaints, how would
>> that memcmp work?
>>
>> Is it that the corresponding .size would always be 0 if .ptr is NULL
>> that protects us?
>
> If we want to be more defensive, we could do something like this:
>
> /* dual inline/macro magic to avoid evaluating ptr twice but knowing
> * enough about the type of *ptr to get the size. */
> #define SAFE_END_PTR(ptr, len) safe_end_ptr(ptr, len, sizeof(*ptr))
> static inline void *safe_end_ptr(void *ptr, size_t nr, size_t elem_size)
> {
> if (!ptr) {
> if (nr)
> BUG("non-zero size coupled with NULL pointer");
> return NULL;
> }
> return (char *)ptr + nr * elem_size;
> }
>
> ...
> char *ap = SAFE_END_PTR(a->ptr, a->size);
>
> I'm not sure if it's worth it, though.
As long as we make it clear to those who add new callers that
any mmfile_t with .ptr==NULL must come with .size==0, it is fine.
> Yet another alternative is to consider it a bug to use an mmfile_t with
> a NULL pointer, figure out where that's being set up, and fix it.
But that would still require us to make it clear to those who add
new callers that mmfile_t with .ptr==NULL is a bug, and the current
callers must be using that as it is convenient for them, I presume,
so I think a simple comment should probably be sufficient.
> As an aside, I also wondered whether we could run into problems with
> "memcmp(NULL, ..., 0)", which is also undefined behavior. But we don't
> here because the first half of the while() condition wouldn't trigger.
Yes, although the details slightly differ ;-)
What is problematic actually is "memcmp(NULL - 1024, ..., 1024)",
which is guarded with "1024 + trimmed <= smaller &&" that will never
be true as long as "mmfile_t with .ptr==NULL must have .size==0"
holds true, right?
Thanks.
