[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ
From: |
Adhemerval Zanella |
Subject: |
Re: [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970] |
Date: |
Wed, 30 Dec 2020 09:34:14 -0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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).
- [PATCH v3 1/6] Import idx.h from gnulib, (continued)
- [PATCH v3 1/6] Import idx.h from gnulib, Adhemerval Zanella, 2020/12/29
- [PATCH v3 2/6] Import filename.h from gnulib, Adhemerval Zanella, 2020/12/29
- [PATCH v3 3/6] malloc: Add scratch_buffer_dupfree, Adhemerval Zanella, 2020/12/29
- [PATCH v3 5/6] support: Add support_small_thread_stack_size, Adhemerval Zanella, 2020/12/29
- [PATCH v3 6/6] stdlib: Add testcase fro BZ #26241, Adhemerval Zanella, 2020/12/29
- [PATCH v3 4/6] stdlib: Sync canonicalize with gnulib [BZ #10635] [BZ #26592] [BZ #26341] [BZ #24970], Adhemerval Zanella, 2020/12/29