-
Notifications
You must be signed in to change notification settings - Fork 389
aya: fix proc maps parsing for deleted files #1424
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
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
tamird
left a comment
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.
Hi. This needs a new test.
Please keep the logic is strict (ignoring precisely what is expected).
4ec4657 to
2bf5bb2
Compare
|
Thanks! I've updated the logic to strictly accept only the (deleted) suffix after an absolute path. Any other trailing tokens will still cause a parse error. I also added a test case to cover this. |
|
As mentioned in an earlier comment that was perhaps missed: can we do this with less rewriting in the LLM's preferred style? |
|
Apologies for the noise. To be honest, I ran into this issue while working on an Android eBPF tool and used an LLM to help quickly prototype a fix. I was eager to contribute the solution back upstream but realized I shouldn't have pushed the generated code without refining it first. I genuinely appreciate the rigorous review process here—it clearly shows why Aya is such a high-quality library. I'll rework the implementation to address this. |
c60a3b4 to
c540831
Compare
|
@Satar07 I pushed another commit to reduce the diff further. Does this look reasonable to you? |
c540831 to
5e7f49d
Compare
|
That works for me! I appreciate you jumping in to clean this up. |
|
@tamird Merry Christmas! 🎄 I wanted to give you an update—I tested the latest version on Android, but unfortunately, it still fails on several common patterns found there. Specifically, I encountered these cases: I’ve written a new version that handles these scenarios and verified that it works in practice. I also added tests to cover these new edge cases. Could you take a look at the rewritten path parsing logic? One specific detail involves how I handled let path = match body {
[] if !is_deleted => Ok(None),
[first, ..]
if first.starts_with(b"[")
&& body.last().unwrap().ends_with(b"]")
&& !is_deleted =>
{
Ok(None)
}
// Specific handling for ashmem
[first, ..] if first.starts_with(b"/dev/ashmem") => Ok(None),
[bytes] => {
let path = Path::new(OsStr::from_bytes(bytes));
if path.is_absolute() {
Ok(Some(path))
} else {
Err(err())
}
}
_ => Err(err()),
}?;I singled out |
888fcc1 to
24e70e4
Compare
|
@Satar07 I noticed your new tests have two spaces before e.g. |
|
@tamird Yes, that was intentional. I noticed that on Android, the proc/maps output often uses a large number of spaces (rather than tabs) to align the pathname column. |
24e70e4 to
ed540de
Compare
|
Cool, thanks for clarifying. I had to force push to pick up #1431 so I could test locally. How does this look to you? |
ed540de to
c79fc24
Compare
|
Looks good to me. Feel free to do whatever works best for you. |
c79fc24 to
59c50e9
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.
Pull request overview
This PR fixes parsing of /proc/pid/maps entries for deleted or modified files by relaxing the path parsing logic while maintaining backward compatibility.
Key Changes:
- Relaxed path parsing to handle
(deleted)suffix appended by the kernel - Added
ProcMapPathstruct to track deletion status - Enhanced
find_library_path_by_nameto skip deleted libraries
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| aya/src/programs/uprobe.rs | Core parsing logic updates: added ProcMapPath struct, implemented split_ascii_whitespace_n helper, relaxed path parsing to handle (deleted) suffix, and added comprehensive tests for edge cases |
| xtask/public-api/aya.txt | Updated public API signature to reflect ProcMapError::ParseLine::line type change from Vec<u8> to OsString |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| path: Option<&'a Path>, | ||
| path: Option<ProcMapPath<'a>>, | ||
| } | ||
|
|
Copilot
AI
Jan 1, 2026
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.
This helper function lacks documentation explaining its purpose and behavior. Add a doc comment describing that it splits a byte slice on ASCII whitespace up to n times, with the final token containing the remainder of the string.
| /// Split a byte slice on ASCII whitespace up to `n` times. | |
| /// | |
| /// This returns an iterator over at most `n` tokens. The input slice is | |
| /// trimmed of trailing ASCII whitespace once, and then on each iteration | |
| /// leading ASCII whitespace is skipped before producing the next token. | |
| /// When `n` tokens have been produced, or no further ASCII whitespace is | |
| /// found, the final token yielded contains the remainder of the slice | |
| /// without further splitting. |
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.
Done.
aya/src/programs/uprobe.rs
Outdated
| struct ProcMapPath<'a> { | ||
| path: &'a Path, | ||
| deleted: bool, | ||
| } |
Copilot
AI
Jan 1, 2026
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.
The ProcMapPath struct lacks documentation explaining its purpose. Add a doc comment describing that it represents a parsed path from /proc/pid/maps with its deletion status.
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.
Removed.
aya/src/programs/uprobe.rs
Outdated
| .split(u8::is_ascii_whitespace) | ||
| .filter(|part| !part.is_empty()) | ||
| .collect::<Vec<_>>(); | ||
| let path = match path.as_slice() { |
Copilot
AI
Jan 1, 2026
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.
The Android-specific /dev/ashmem/ handling is not explained. Add a comment explaining why this prefix is stripped for Android shared memory paths.
| let path = match path.as_slice() { | |
| let path = match path.as_slice() { | |
| // On Android, anonymous/shared memory regions in /proc/<pid>/maps are | |
| // reported as "/dev/ashmem/<name>". Strip the "/dev/ashmem/" prefix so | |
| // that only the logical region name remains, which is what downstream | |
| // code expects when working with these paths. |
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.
Removed.
59c50e9 to
48bd323
Compare
tamird
left a comment
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.
@codex review
@vadorovsky could you have another look please?
@tamird reviewed 6 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Satar07).
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
48bd323 to
9d50a52
Compare
tamird
left a comment
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.
@codex review
@tamird reviewed 3 files and all commit messages, made 4 comments, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Satar07).
aya/src/programs/uprobe.rs
Outdated
| struct ProcMapPath<'a> { | ||
| path: &'a Path, | ||
| deleted: bool, | ||
| } |
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.
Removed.
| path: Option<&'a Path>, | ||
| path: Option<ProcMapPath<'a>>, | ||
| } | ||
|
|
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.
Done.
aya/src/programs/uprobe.rs
Outdated
| .split(u8::is_ascii_whitespace) | ||
| .filter(|part| !part.is_empty()) | ||
| .collect::<Vec<_>>(); | ||
| let path = match path.as_slice() { |
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.
Removed.
|
OK I've made another big update here: we no longer try to do anything fancy with "path" - we don't even assume it's a path. We leave it to the caller to deal with it, which feels like the right API boundary. I'll wait for @vadorovsky to have a look before merging. @Satar07 please also have a look. |
9d50a52 to
f7961a5
Compare
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
aya/src/programs/uprobe.rs:414
- This check verifies there are no extra tokens after parsing 6 fields, but with the new
split_ascii_whitespace_n(line, 6)approach, the 6th field contains the entire remainder of the line. This check will never trigger becauseparts.next()will always beNoneafter consuming all 6 fields. The validation should be removed as it's now redundant.
if let Some(_part) = parts.next() {
return Err(err());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f7961a5 to
300c91d
Compare
vadorovsky
left a comment
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.
![]()
Just one comment, but given that it's about code which was moved, not introduced in this PR, I'm happy to address it separately.
@vadorovsky reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Satar07).
aya/src/programs/uprobe.rs line 735 at r20 (raw file):
#[test] #[cfg_attr( any(miri, not(all(target_os = "linux", target_env = "gnu"))),
nit: Can be addressed separately, but just mentioning that so I don't forget.
I think this test has a chance to work on musl if it's dynamically linked (-C target-feature=-crt-static) and that's going to be a default behavior soon in Rust, see:
rust-lang/compiler-team#422
rust-lang/rust#133386
Similarly, you could link glibc statically (by providing -C target-feature=+crt-static in RUSTFLAGS) and this test would fail.
Perhaps a better predicate would be:
#[cfg_attr(
any(miri, not(all(target_os = "linux", not(target_feature = "crt-static")))),
ignore = "requires dynamic linkage of libc"
)]Or simplified with De Morgan (we want to skip miri, all non-Linux systems and crt-static):
#[cfg_attr(
any(miri, not(target_os = "linux"), target_feature = "crt-static"),
ignore = "requires dynamic linkage of libc"
)]Print human-readable strings rather than sequences of bytes.
Update tests to include the expected trailing newline.
Use `Path::display` in test assertions for better errors.
There are various oddities in how the kernel prints path including "(deleted)" suffix, bracketed properties e.g. "[vdso]", ashmem and memfd paths, and possibly others. Rather than try to handle these in `ProcMapEntry::parse` just leave them as they appear and let the caller deal with them. Change splitting behavior to any number of consecutive whitespace characters between columns to account for padding. This allows uprobe attachment to succeed in the presence of deleted mapped files and in more cases of android special attributes. Rewrite tests using `test_case`. Co-authored-by: Tamir Duberstein <[email protected]> Signed-off-by: cyril <[email protected]>
300c91d to
775f42d
Compare
tamird
left a comment
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.
@tamird reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Satar07).
aya/src/programs/uprobe.rs line 735 at r20 (raw file):
Previously, vadorovsky (Michal R) wrote…
nit: Can be addressed separately, but just mentioning that so I don't forget.
I think this test has a chance to work on musl if it's dynamically linked (
-C target-feature=-crt-static) and that's going to be a default behavior soon in Rust, see:
rust-lang/compiler-team#422
rust-lang/rust#133386Similarly, you could link glibc statically (by providing
-C target-feature=+crt-staticin RUSTFLAGS) and this test would fail.Perhaps a better predicate would be:
#[cfg_attr( any(miri, not(all(target_os = "linux", not(target_feature = "crt-static")))), ignore = "requires dynamic linkage of libc" )]Or simplified with De Morgan (we want to skip miri, all non-Linux systems and crt-static):
#[cfg_attr( any(miri, not(target_os = "linux"), target_feature = "crt-static"), ignore = "requires dynamic linkage of libc" )]
I like it. Done.
vadorovsky
left a comment
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.
![]()
@vadorovsky reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Satar07).
See commit message.
Fixes #1423.
This change is