Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970]

[Adhemerval Zanella]

On 30/12/2020 09:34, Adhemerval Zanella wrote:
> 
> 
> On 29/12/2020 22:21, Paul Eggert wrote:
>> On 12/29/20 11:34 AM, Adhemerval Zanella wrote:
>>>                idx_t len = strlen (end);
>>> +              if (INT_ADD_OVERFLOW (len, n))
>>> +                {
>>> +                  __set_errno (ENAMETOOLONG);
>>> +                  goto error_nomem;
>>> +                }
>>
>>
>> The other patches in this glibc patch series look good to me. However, this 
>> patch has some problems. First, the overflow check does not handle the case 
>> where strlen (end) does not fit into len. Second, ENAMETOOLONG is not the 
>> right errno; it should be ENOMEM because not enough memory can be allocated 
>> (this is what scratch_buffer, malloc, etc. do in similar situations). Third 
>> (and less important), the overflow check is not needed on practical 64-bit 
>> platforms either now or in the forseeable future.
>>
>> I installed the attached patch into Gnulib to fix the bug in a way I hope is 
>> better. The idea is that you should be able to sync this into glibc without 
>> needing a patch like the above.
>>
> 
> Indeed, the test which triggered only failed on 32-bits platforms
> and it uses a exactly INT_MAX size to trigger it. I agree that
> it should not be a problem for 64-bit, however I don't think there is
> much gain is adding the NARROW_ADDRESSES trick: it makes the code 
> conditionally build depending of the idx_t size and it is just really 
> a small optimization that adds code complexity on a somewhat convoluted 
> code.
> 
> For ENAMETOOLONG, I think this is the right error code: it enforces
> that we do not support internal objects longer that PTRDIFF_MAX.
> The glibc now enforces is through malloc functions and we haven't
> done it through mmap because if I remember correctly Carlos has pointed
> out some esoteric use case by some projects that allocated 2GB plus
> some slack of continuous memory for 32-bit (I really want to start
> enforce on mmap as well, maybe I will send a patch for new glibc version).
> I think it should be a fair assumption to make it on internal code, such 
> as realpath (this is another reason why I think NARROW_ADDRESSES is not 
> necessary).
> 

And your fix (from 93e0186d4) does not really solve the issue, since
now that len is a size_t the overflow check won't catch the potentially
allocation larger than PTRDIFF_MAX (the realpath will still fail with
ENOMEM though).

Wouldn't the below be simpler?

              size_t len = strlen (end);
              if (len > IDX_MAX || INT_ADD_OVERFLOW ((idx_t) len, n))
                {
                  __set_errno (ENAMETOOLONG);
                  goto error_nomem;
                }