bug-gnulib
[Top][All Lists]
Advanced

[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: Mon, 4 Jan 2021 09:52:10 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0


On 01/01/2021 21:04, Paul Eggert wrote:
> On 12/30/20 5:10 AM, Adhemerval Zanella wrote:
> 
>>> it is just really
>>> a small optimization that adds code complexity on a somewhat convoluted
>>> code.
> 
> The code is indeed simpler without the NARROW_ADDRESSES optimization, so I 
> removed that optimization by installing the attached patch into Gnulib.
> 
>>> For ENAMETOOLONG, I think this is the right error code: it enforces
>>> that we do not support internal objects longer that PTRDIFF_MAX.
> 
> This sounds backwards, as the code returns ENOMEM every other place it tries 
> to create an internal object longer than PTRDIFF_MAX - these ENOMEM checks 
> are in the malloc calls invoked by scratch_buffer_grow and 
> scratch_buffer_grow_preserve. It would be odd for canonicalize_file_name to 
> return ENAMETOOLONG for this one particular way of creating a too-large 
> object, while at the same time it returns ENOMEM for all the other ways.
> 
> Besides, ENAMETOOLONG is the POSIX error code for exceeding NAME_MAX or 
> PATH_MAX, which is not what is happening here.> 
> In Gnulib and other GNU apps we've long used the tradition that ENOMEM means 
> you've run out of memory, regardless of whether it's because your heap or 
> your address space is too small. This is a good tradition and it'd be good to 
> use it here too.

Right, I think we can now assume that that since the  implementation does 
not really imposes any NAME_MAX or PATH_MAX limitations, returning memory
allocation errors instead of ENAMETOOLONG is ok.  I will adjust the 
stdlib/test-bz22786.c, it will require a slight large runtime and memory
requirements (which should be ok).
> 
>>> I think it should be a fair assumption to make it on internal code, such
>>> as realpath
> 
> Yes, staying less than PTRDIFF_MAX is a vital assumption on internal objects. 
> I'd go even further and say it's important for user-supplied objects, too, as 
> so much code relies on pointer subtraction and we can't realistically 
> prohibit that within glibc.

We do enforce it through memory allocations, however users can still craft
such objects using mmap (some libc does imposes the same PTRDIFF_MAX limit
on mmap as well).

> 
>> (this is another reason why I think NARROW_ADDRESSES is not necessary).
> 
> Unfortunately, if we merely assume every object has at most PTRDIFF_MAX 
> bytes, we still must check for overflow when adding the sizes of two objects. 
> The NARROW_ADDRESSES optimization would have let us avoid that unnecessary 
> check on 64-bit machines.
> 
>> 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).
> 
> Sure, which means the code is doing the right thing: it's failing with ENOMEM 
> because it ran out of memory. There is no need for an extra PTRDIFF_MAX check 
> in canonicalize.c if malloc (via scratch_buffer_grow) already does the check.
>> 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;
>>                  }
> 
> It's not simpler than the attached Gnulib patch, because it contains an 
> unnecessary comparison to IDX_MAX and an unnecessary cast to idx_t.

The extra comparison might avoid the scratch_buffer resize that will
fail (since malloc will fail to try allocate PTRDIFF_MAX object), but
it will be used only when such objects are provided (which depending
of the system should not happen).

Thanks, I synced with the most recent gnulib version. 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]