-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent crash with /proc path canonicalization #1637
Conversation
8c8cee7
to
ac052ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add another test that exercises the previously broken behavior?
src/utils.cpp
Outdated
// filesystem::canonical does not work very well with /proc paths | ||
// failing during canonicalization. See iovisor:bpftrace#1595 | ||
if (rel_path.rfind("/proc", 0) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the issue isn't that fs::canonical() doesn't work on procfs [0], it's that we're trying to canonicalize across mount namespaces. And part of the issue is that /proc/pid/root
is a symlink to target pid's root as viewed by the target.
Wouldn't a better fix be to check for /root
as well as /proc
?
[0]: http://ix.io/2EQ0 prints "/usr/bin"
for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have clarified the comment, limited the check for /proc/<pid>/root
, and improved the test to cover the functions behaviour.
I assume by "test that exercises the previously broken behavior" you meant a test that actually tries to trace with a path of a process from another mount ns? That test already exists but is disabled, I have created an issue to re-enable it: #1638
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume by "test that exercises the previously broken behavior" you meant a test that actually tries to trace with a path of a process from another mount ns? That test already exists but is disabled, I have created an issue to re-enable it: #1638
Nice :)
5e2d975
to
102e8c0
Compare
src/utils.cpp
Outdated
// filesystem::canonical does not work very well with /proc/<pid>/root paths | ||
// of processes in a different mount namespace (than the one bpftrace is | ||
// running in), failing during canonicalization. See iovisor:bpftrace#1595 | ||
if (!std::regex_match(rel_path, std::regex("^/proc/\\d+/root/.*"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be expensive to keep compiling regex. Let's just do it once.
ie:
static auto re = std::regex("^/proc/\\d+/root/.*");
if (!std::regex_match(rel, path, re))
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
102e8c0
to
a863f26
Compare
The filesystem::canonical does not work very well with /proc paths, explictly ignoring those paths from filesystem::canonical for now.
a863f26
to
231b6fd
Compare
LGTM |
Thanks for the fix! |
* 'master' of github.com:iovisor/bpftrace: Prevent crash with /proc path canonicalization (bpftrace#1637)
Issue
Attempting to trace an executable (with either namespace relative, or /proc rooted path) that is in a different mount namespace than the one bpftrace is running in fails.
Cause
The reason is attempting to perform
filesystem::canonical('/proc/<pid>/root/usr/bin/..')
to a proc path of a pid that is in a different mount namespace fails withNo such file or directory
. (it looks like canonical() does an lstat on the current root instead and doesn't find the file there)Call stack
The call originates from
abs_path
, and this is the call stack before it blows up:Fix
The fix is to give up trying to canonicalize
/proc
based baths. That of course means something like tracing/proc/92493/root/home/../home/usdt_test
will not work (it doesn't event work now anyway), but I don't see a strong use case for trying to support canonicalization of /proc based paths.Checklist
CHANGELOG.md