Page MenuHomePhabricator

[Support] Treat truncation of fullpath as error
ClosedPublic

Authored by Hahnfeld on Mar 1 2019, 10:26 AM.

Details

Summary

If the concatenation of arguments dir and bin has at least PATH_MAX
characters the call to snprintf will truncate. The result will usually
not exist, but if it does it's actually incorrect to return that the
path exists.
(Motivated by GCC compiler warning about format truncation.)

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld created this revision.Mar 1 2019, 10:26 AM
mgrang edited reviewers, added: efriedma; removed: eli.friedman.Mar 1 2019, 11:46 AM
efriedma added inline comments.Mar 1 2019, 12:36 PM
llvm/lib/Support/Unix/Path.inc
114 ↗(On Diff #188938)

The use of PATH_MAX here seems dubious; in practice, many targets support paths longer than PATH_MAX. Probably should just use a raw_string_ostream or something like that instead of snprintf to a fixed buffer.

zturner added inline comments.
llvm/lib/Support/Unix/Path.inc
114 ↗(On Diff #188938)

Even better, llvm::sys::path::join(). I'm not sure why all of these global functions operate on raw pointers instead of LLVM's string ADTs.

Hahnfeld marked 3 inline comments as done.Mar 3 2019, 12:44 PM
Hahnfeld added inline comments.
llvm/lib/Support/Unix/Path.inc
114 ↗(On Diff #188938)

I played a bit on my Arch Linux system: It's indeed possible to create paths that are longer than PATH_MAX = 4096. But I can't get the realpath nor even cd into there, and I'd guess that many more tools are broken.

In that case I think there's no difference between fixed buffers and the string ADTs, except that the latter are probably slower.

Ping. As I said I think the current approach with a fixed-size buffer makes sense because anything larger likely won't work anyhow.

efriedma accepted this revision.Mar 12 2019, 11:34 AM

LGTM

llvm/lib/Support/Unix/Path.inc
114 ↗(On Diff #188938)

I guess this is okay given the way the code is currently written.

It should probably be rewritten eventually to work in a more consistent way on systems which don't have a PATH_MAX, like Hurd, but that shouldn't block this patch, I think.

This revision is now accepted and ready to land.Mar 12 2019, 11:34 AM
This revision was automatically updated to reflect the committed changes.