-
Notifications
You must be signed in to change notification settings - Fork 286
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
uprobe,integration-test: do not exercise dylib resolution in integration tests #717
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ajwerner)
-- commits
line 6 at r1:
"in anticipation"
-- commits
line 13 at r2:
"isn't part of" is an odd wording. Isn't referenced from?
-- commits
line 17 at r2:
how could that oddity be addressed? the bit about the absolute path? i think there's a nicer way to phrase what this commit does or does not do.
.github/workflows/ci.yml
line 231 at r2 (raw file):
build-workflow-complete: needs: ["lint", "build-test-aya", "build-test-aya-bpf", "run-integration-test"]
seems unrelated
aya/src/programs/uprobe.rs
line 126 at r1 (raw file):
} // Look up the path for the target. If it there is a pid, and the target is a library name
this commentary may be better placed inside the body; it describes the implementation rather than the contract.
aya/src/programs/uprobe.rs
line 133 at r1 (raw file):
) -> Result<String, UProbeError> { let target = target.as_ref(); let target_str = &*target.as_os_str().to_string_lossy();
there are several unnecessary conversions and allocations happening here. I realize you're just moving code around, so feel free to disregard.
aya/src/programs/uprobe.rs
line 166 at r1 (raw file):
// be dynamically linked to libc and can exercise resolving the path to libc via the current // process's memory map. #[cfg(all(target_os = "linux", target_env = "gnu"))]
we should compile this test always, but only run it on linux-gnu.
#[cfg_attr(not(all(target_os = "linux", target_env = "gnu")), ignore = "something descriptive goes here"]
aya/src/programs/uprobe.rs
line 174 at r1 (raw file):
// Now let's resolve the path to libc. It should exist in the current process's memory map and // then in the ld.so.cache. let libc_path = resolve_attach_path("libc", Some(pid.try_into().unwrap())).unwrap();
nit: have mercy on the eyes.
let pid = pid.try_into().unwrap();
let libc_path = resolve_attach_path("libc", Some(pid)).unwrap();
test/integration-test/src/tests/load.rs
line 432 at r2 (raw file):
#[test] fn pin_lifecycle_uprobe() { const INITIAL_PIN_PATH: &str = "/sys/fs/bpf/aya-uprobe-test-prog";
how about FIRST and a "-1" suffix for symmetry?
81cce9c
to
de1deb8
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.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @tamird)
Previously, tamird (Tamir Duberstein) wrote…
"in anticipation"
Done.
Previously, tamird (Tamir Duberstein) wrote…
"isn't part of" is an odd wording. Isn't referenced from?
Done.
Previously, tamird (Tamir Duberstein) wrote…
how could that oddity be addressed? the bit about the absolute path? i think there's a nicer way to phrase what this commit does or does not do.
Done.
.github/workflows/ci.yml
line 231 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
seems unrelated
Done.
aya/src/programs/uprobe.rs
line 126 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
this commentary may be better placed inside the body; it describes the implementation rather than the contract.
Done.
aya/src/programs/uprobe.rs
line 166 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
we should compile this test always, but only run it on linux-gnu.
#[cfg_attr(not(all(target_os = "linux", target_env = "gnu")), ignore = "something descriptive goes here"]
Done.
aya/src/programs/uprobe.rs
line 174 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
nit: have mercy on the eyes.
let pid = pid.try_into().unwrap(); let libc_path = resolve_attach_path("libc", Some(pid)).unwrap();
Done.
test/integration-test/src/tests/load.rs
line 432 at r2 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
how about FIRST and a "-1" suffix for symmetry?
Done.
de1deb8
to
9ce8596
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.
Reviewed 3 of 3 files at r3, 2 of 2 files at r4, 3 of 3 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ajwerner)
aya/src/programs/uprobe.rs
line 166 at r1 (raw file):
Previously, ajwerner wrote…
Done.
you missed the "not". maybe put miri first: any(miri, not(all(...)))
9ce8596
to
69aa681
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.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @tamird)
aya/src/programs/uprobe.rs
line 133 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
there are several unnecessary conversions and allocations happening here. I realize you're just moving code around, so feel free to disregard.
aya/src/programs/uprobe.rs
line 166 at r1 (raw file):
Previously, tamird (Tamir Duberstein) wrote…
you missed the "not". maybe put miri first:
any(miri, not(all(...)))
Done.
@ajwerner, this pull request is now in conflict and requires a rebase. |
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.
FYI this overlaps with #719 some, but I think we should land it to unblock VM testing.
Reviewed 2 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ajwerner)
The function is extracted so that a test could be written. This test is valid on linux-gnu targets, and it doesn't need any special privileges. This is in anticipation of removing the code that uses this functionality (seemingly incidentally) from integration tests.
69aa681
to
5b99407
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.
Reviewed 2 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ajwerner)
For unclear reasons, two of the integration tests related to uprobes were resolving a symbol in libc. The integration-test binary can be built statically, in which case it would not load or reference libc. Statically linking the integration tests and running them in a VM without a userland is a convenient mechanism to exercise the tests against different kernel versions. The fact that the statically linked integration-test binary does not load libc is not the only reason these tests failed in such an environment. In fact, the logic to look in the process's memory maps was not running (because no pid was being passed). Separate logic to determine which object file to use when attempting to resolve a symbol for attaching a uprobe changes its behavior based on whether that target is an absolute path. If the target is not an absolute path, the code searches through the LdSoCache. This cache does not always exist in linux systems; when an attach call is made with a relative path target and there is no /etc/ld.so.cache file, the attach call will fail. This commit does not change that behavior, it merely sidesteps it.
5b99407
to
79c9489
Compare
See individual commits.
This change is