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