Re: [PATCH 1/6] canonicalize-lgpl: fix EOVERFLOW bug

[Adhemerval Zanella]

On 18/12/2020 09:30, Adhemerval Zanella wrote:
> 
> 
> On 17/12/2020 07:55, Paul Eggert wrote:
>> On 12/11/20 5:03 AM, Adhemerval Zanella wrote:
>>
>>> I have sent a similar fix to reviews for this very issue for glibc
>>> (which is based on the canonicalize-lgpl) along with other fixes that
>>> you might take a look at [1].
>>
>> Thanks, I looked at that when composing the patches I just now installed 
>> into Gnulib, which also attempt to make future glibc merges easier. I think 
>> these patches address the issues mentioned in [1] (though sometimes in 
>> different ways), except they don't do what this patch does:
>>
>> https://patchwork.sourceware.org/project/glibc/patch/20201027143531.2448132-4-adhemerval.zanella@linaro.org/
>>
>> This patch seems incomplete, since it doesn't address the issue that 
>> canonicalize_file_name ("/a/b/.") incorrectly returns "/a/b" even when /a/b 
>> is a regular file.
> 
> I have suggested using scratch_buffer on some previous iterations a while
> back [1], and although it was incomplete it would be good to had some
> feedback back then to work towards a better solution. I dropped the 
> scratch_buffer on the next version [2] because it resulted in a simplified
> code with static stack usage.
> 
> Anyway, sometimes I feel glibc is a disjointed project that do not usually
> work together with gnulib, even though it shared a lot of code.  I would 
> expect that instead of gnulib to just drop a large patch, we would work 
> together toward a better solution that suits both projects. Yes, I know 
> glibc development side might be slow at times; and maybe it would be better
> if I had sent the patch on gnulib first. But this kind of development where 
> glibc side is somewhat ignored makes me wonder if sharing code with gnulib 
> is really beneficial.
> 
>>
>> Come to think of it, the current Gnulib is suboptimal in that it uses stat 
>> ("/a/b/foo/", ...) to test for the existence of a directory /a/b/foo. It 
>> could use faccessat (AT_FDCWD, "/a/b/foo/", F_OK, AT_EACCESS), which is 
>> often cheaper as it needn't fill in the stat structure.
>>
>> I'll try to make time to look into these two issues, which are somewhat 
>> related in the implementations of canonicalize-lgpl and canonicalize.
> 
> This is exactly what I am trying to avoid on my latest patch on the
> glibc series.  It might be not on par of gnulib code quality standard,
> but it might give you some ideas on how to remove the stat call.
> 
> I will drop my patchset and sync with gnulib code. It should at least
> fix BZ #26592 and BZ #26241 and I will work to add the faulty testcase
> you mentioned.  The BZ #24970 is mostly a optimization, so we can postpone
> after 2.33 release.
> 
> [1] https://sourceware.org/pipermail/libc-alpha/2020-August/117068.html
> [2] https://sourceware.org/pipermail/libc-alpha/2020-September/117522.html
> [3] 
> https://patchwork.sourceware.org/project/glibc/patch/20201027143531.2448132-4-adhemerval.zanella@linaro.org/
> 

The same tests I pointed out on BZ#24970 comment #2 still fails with gnulib
version 0aa8ef424.  I am not sure if it would be better to adapt my original 
patchset to use scratch_buffer (which seems originally a better idea) or if 
we can work towards fixing on gnulib. 

[1] 
https://patchwork.sourceware.org/project/glibc/patch/20201027143531.2448132-1-adhemerval.zanella@linaro.org/