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

[Adhemerval Zanella]

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