Re: [PATCH 4/5] strbuf_readlink(): support link targets that exceed PATH_MAX
Patrick Steinhardt <ps@pks.im> writes:
Show 28 quoted lines
>> > This makes me wonder whether we have a better way to figure out the
>> > actual size of the buffer that we ultimately need to allocate. But
>> > reading through readlink(3p) doesn't indicate anything, and I'm not sure
>> > whether we can always rely on lstat(3p) to return the correct size for
>> > symlink contents on all platforms.
>> >
>> > One thing that _is_ noted though is that calling the function with a
>> > buffer size larger than SSIZE_MAX is implementation-defined. It does
>> > make me a bit uneasy in that light to grow indefinitely.
>> >
>> > Which makes me wonder whether Windows has a limit for the symlink
>> > contents that we could enforce in theory so that we can reasonably turn
>> > this into a bounded loop again?
>>
>> https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
>> suggests that the maximum permissible target path should be 32,768. But
>> that's not _quite_ correct, as
>> `../t/../Documentation/RelNotes/../../README.md` is a perfectly valid (if
>> awkward) symlink target.
>>
>> Still, I would say that 32,768 would make for a fine (still insanely high,
>> but not so high as to allow malicious symlinks to cause memory problems)
>> limit.
>>
>> Sound good?
>> Johannes
>
> Sounds good to me, thanks!
As this is a generic codepath in strbuf.c, platforms that do not honor Microsoft's promise cited above can break the assumption made here by going beyond 32k, no?
I am OK if this infinite loop had our own "we are growing the buffer very long and still getting not-enough-buf error; let's give up" termination condition.
IOW, a simpler alternative may be
---- >8 ----
Subject: strbuf_readlink(): do not trust PATH_MAX
We have been bitten before by platforms that sets PATH_MAX way too low, far below the length of paths they comfortably support. The strbuf_readlink() limits the link targets to PATH_MAX, which is a code path that is broken by such platforms.
Raise the limit to 32kB, which matches the limit of a platform with such a problem [*].
* https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
strbuf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to diff +5 −1
diff --git c/strbuf.c w/strbuf.c
index 7fb7d12ac0..1c7659bcd2 100644
--- c/strbuf.c
+++ w/strbuf.c
@@ -566,7 +566,11 @@ ssize_t strbuf_write(struct strbuf *sb, FILE *f)
return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
}
-#define STRBUF_MAXLINK (2*PATH_MAX)
+/*
+ * Do not use PATH_MAX, as some platforms sets it too low;
+ * 32kB matches what Windows has as the real limit for a pathnname.
+ */
+#define STRBUF_MAXLINK (2 * (1 << 15))
int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint)
{
Johannes Schindelin via GitGitGadget
