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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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;
                }