[cyphar]

> - There's a lovely comment in syscall.c:1660-1673 that's quite bad. It's handling strings that contain "/../" and such. If there's some actual contract that the function makes to its callers (and there surely is -- this is critical security-sensitive code), then SAY WHAT THE CONTRACT IS. Don't bury a partial explanation in a a comment in the middle.

I took a look at this earlier before seeing your comment and (as the author of openat2), I found a few other bugs[1] that you could easily imagine coming out of Claude if you didn't religiously review every edit. My impression is that the (unnecessary) ".." handling was added by Claude to provide "consistency" but it actually doesn't make sense if you think about it for more than a few minutes.

I will say that I don't really blame Tridge for this, Claude often writes code like this and it is very hard to notice it making dumb assumptions unless you sit back and read the code as if it was submitted to you by someone else without any context from the conversation with the LLM. It is very hard to switch from "System 1" thinking (talking with an LLM) to "System 2" thinking (reviewing the code as if someone you do not trust submitted it)[2].

This is one of the reasons I only really use LLMs for code review or writing tests to see if it can find edge cases I didn't see -- it's really hard to be sure it didn't make a reasonable-sounding-but-actually-remarkably-stupid assumption when writing code that is not something you are intimately familiar with. In those cases, actually writing the code gives you the chance to think about those edge cases and do research based on that, so even if you buy the argument that LLMs can output similar code to what you would've written (which hasn't been my experience at all) it is very hard to review changes without that background knowledge.

[1]: https://github.com/RsyncProject/rsync/pull/939 [2]: https://en.wikipedia.org/wiki/Thinking%2C_Fast_and_Slow

[amluto]

Hah, you read more of it than I did :) I’m willing to believe that there is some other code path that thinks that a path like "a/../b" is not allowable, and Claude saw that and wanted to enforce it, and then forgot about it when writing the rest of its code. I didn’t try to dig in to the history, nor do I want to.

(Of course, catching "a/../b" is not the same as catching "a/foo/b" where foo is a symlink to "..". Claude doesn’t think unless you make it think.)

(Hi, cyphar!)

[cyphar]

> I’m willing to believe that there is some other code path that thinks that a path like "a/../b" is not allowable, and Claude saw that and wanted to enforce it, and then forgot about it when writing the rest of its code.

That is also possible, but this mostly smells like a weird LLM-hallucinated assumption to me because the whole concept of blocking literal ".." components doesn't make sense if you allow symlinks -- symlinks can contain ".." and RESOLVE_BENEATH allows them if they are safe. Maybe the old code did have that faulty assumption built in (there were some security bugs related to it, after all), but one would hope that The $1T A.G.I.(TM) would be able to notice such a mistake.

EDIT: Ah, you added a note about ".." and symlinks. Yeah, that was one of the major things that made me think the whole thing was an LLM hallucination or possibly it over-applying a critique that Tridge may have made when reviewing the code. Without access to the chat logs it's hard to know.

(For completely unrelated reasons, I plan to send a patch for RESOLVE_NO_DOTDOT that lets you block them entirely, so you could eventually do this kind of blocking if you really wanted to but most users would prefer RESOLVE_BENEATH behaviour anyway -- blocking ".." entirely will lead to very painful regressions, as other users have already mentioned.)

(PS: Hey Andy, long time no see! :D)