Re: [PATCH] strbuf: support long paths w/o read rights in strbuf_getcwd() on FreeBSD

From: René Scharfe <l.s.r@web.de>
Date: Mar 30, 2017, 18:01 UTC
Message-ID: <4a25d1f4-a744-9135-93af-7e675680b91e@web.de>
In-Reply-To: <CAP8UFD1wqW221qxWqMz1jHxDLKCg71a2VBh+wUdV8sMSEN2aZw@mail.gmail.com>

Am 29.03.2017 um 06:54 schrieb Christian Couder:

Show 40 quoted lines

> On Tue, Mar 28, 2017 at 11:49 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Mar 28, 2017 at 11:15:12PM +0200, Christian Couder wrote:
>>
>>> On Sun, Mar 26, 2017 at 3:43 PM, René Scharfe <l.s.r@web.de> wrote:
>>>> FreeBSD implements getcwd(3) as a syscall, but falls back to a version
>>>> based on readdir(3) if it fails for some reason.  The latter requires
>>>> permissions to read and execute path components, while the former does
>>>> not.  That means that if our buffer is too small and we're missing
>>>> rights we could get EACCES, but we may succeed with a bigger buffer.
>>>>
>>>> Keep retrying if getcwd(3) indicates lack of permissions until our
>>>> buffer can fit PATH_MAX bytes, as that's the maximum supported by the
>>>> syscall on FreeBSD anyway.  This way we do what we can to be able to
>>>> benefit from the syscall, but we also won't loop forever if there is a
>>>> real permission issue.
>>>
>>> Sorry to be late and maybe I missed something obvious, but the above
>>> and the patch seem complex to me compared with something like:
>>>
>>> diff --git a/strbuf.c b/strbuf.c
>>> index ace58e7367..25eadcbedc 100644
>>> --- a/strbuf.c
>>> +++ b/strbuf.c
>>> @@ -441,7 +441,7 @@ int strbuf_readlink(struct strbuf *sb, const char
>>> *path, size_t hint)
>>>  int strbuf_getcwd(struct strbuf *sb)
>>>  {
>>>         size_t oldalloc = sb->alloc;
>>> -       size_t guessed_len = 128;
>>> +       size_t guessed_len = PATH_MAX > 128 ? PATH_MAX : 128;
>>>
>>>         for (;; guessed_len *= 2) {
>>>                 strbuf_grow(sb, guessed_len);
>>
>> I think the main reason is just that we do not have to pay the price to
>> allocate PATH_MAX-sized buffers when they are rarely used.
>>
>> I doubt it matters all that much in practice, though.
>
> Yeah, I can understand that.

Right, using PATH_MAX as initial size (no ?: operator needed) is the easiest fix. And yes, I wanted to avoid wasting memory just to fix an odd special case.

Given that the function isn't called very often the impact is probably not that high either way, but increasing memory usage by 3200% on Linux just didn't sit right with me -- even though 4KB isn't much in absolute terms, of course.

> But I also wonder if René's patch relies on PATH_MAX being a multiple
> of 128 on FreeBSD and what would happen for a path between 129 and
> PATH_MAX if PATH_MAX was something like 254.

We can use any value bigger than zero to initialize guessed_len. getcwd() won't have any problems fitting e.g. a path with a length of 130 bytes into a buffer of 256 bytes in the second round of the loop.

René