aya: return errors instead of panicking on invalid CString inputs by surno · Pull Request #1560 · aya-rs/aya

surno · 2026-05-05T20:04:56Z

Summary

Resolves the six // TODO: avoid this unwrap by adding a new error variant
sites scattered across aya, and aligns the resulting error-variant shape
across the four enums that gained an InvalidPinPath variant. The unwraps
would panic if a caller passed a path or name containing an interior nul
byte; this PR converts each into a typed error. Picks up the work from
#1389 (stalled since April) and applies the review feedback from that
thread.

Approach

Five commits, each scoped to one concern:

aya: return errors instead of panicking on invalid CString inputs
- Pattern A — path → CString (4 sites): added InvalidPinPath { path, source }
  variants to ProgramError, LinkError, and MapError, mirroring the
  existing PinError::InvalidPinPath in aya/src/pin.rs.
- Pattern B — name → CString: RawTracePoint::attach uses a new
  ProgramError::InvalidTracepointName variant. (This site had an
  unmarked .unwrap() not flagged by a TODO, but in the same family —
  caught from Fix TODOs of unwrap calls by implementing Error Variants #1389's coverage.)
aya: report invalid TC name in NulError variant
- Reshapes TcError::NulError from a tuple variant into
  NulError { name, source }, so callers can see which TC name was
  rejected. Updates SchedClassifier::do_attach and
  qdisc_detach_program to populate the new field.
aya: ProgramError::InvalidInterfaceName error variant
- Adds a dedicated ProgramError::InvalidInterfaceName { name, source }
  for Xdp::attach, instead of folding the failure into the pre-existing
  UnknownInterface variant.
aya: rename PinError::InvalidPinPath field error to source
- The pre-existing PinError::InvalidPinPath used error: NulError.
  With the new sibling variants in commits 1–3 settling on
  source: NulError (matching thiserror conventions and Fix TODOs of unwrap calls by implementing Error Variants #1389's review
  feedback), this commit aligns the original to avoid a permanent
  inconsistency across the four parallel InvalidPinPath variants.
aya: include NulError source in MapError::InvalidName
- MapData::create discarded the underlying NulError when the map
  name failed conversion. Adds a source: NulError field to
  MapError::InvalidName so error reporters can walk the chain,
  matching the source-handling pattern used elsewhere in this PR.

SemVer notes

These are breaking changes for downstream callers that exhaustively match
on the affected enums. Per Cargo's SemVer guide
they are technically major, though consistent with how variants are added
elsewhere in this repo:

Adds new variants (InvalidPinPath, InvalidTracepointName,
InvalidInterfaceName) to ProgramError, LinkError, and MapError
(none of which are #[non_exhaustive]).
Reshapes TcError::NulError from a tuple variant to a struct variant.
Renames PinError::InvalidPinPath's field error → source.
Adds a source field to MapError::InvalidName.

cargo xtask public-api snapshot updated accordingly.

Tests

I considered adding unit tests for the new error variants but did not see
precedent for testing similar variants (e.g. PinError::InvalidPinPath,
ProgramError::InvalidName). Happy to add them if you'd like.

Test plan

cargo check
cargo +nightly fmt --all -- --check
cargo clippy -p aya --all-targets -- -D warnings
cargo test -p aya --lib (134 passing)
cargo xtask public-api (snapshot updated)

This change is

netlify · 2026-05-05T20:05:02Z

✅ Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`3672dc8`
🔍 Latest deploy log	https://app.netlify.com/projects/aya-rs-docs/deploys/69fd538508e46600082fd13b
😎 Deploy Preview	https://deploy-preview-1560--aya-rs-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

tamird · 2026-05-05T20:10:21Z

There was a previous attempt at this in #1389. Could you please consider those review comments and confirm that those comments do not apply here? Thanks for your contribution.

surno · 2026-05-05T21:07:05Z

There was a previous attempt at this in #1389. Could you please consider those review comments and confirm that those comments do not apply here? Thanks for your contribution.

Thanks. I went through #1389 and here's how I mapped each comment to this PR:

please call this source instead of e

Addressed. Updated the name from error -> source.

Use PathBuf for path fields (avoid lossy String)

Already done. All path fields use PathBuf.

above this line, please write let path = path.as_ref(); to reduce repetition

Compled. See maps/info:120 , programs/info.rs:227, programs/links:351, programs/mod.rs:623

Squash commits

Already done. Single commit.

can you keep the original error as #[source]?

Completed. Every new variant has #[source] expect the xdp::attach callsite. It puts the NulError into the existing UnknownInterface since a nul-containing wouldn't exist anyways. If you prefer a dedicated variant, I can add that instead.

add NulError to the imports at the top plz

Kept the fully qualified name to keep the diff scoped. I can update the call sites and drop the fully qualified name at each point, if you prefer.

please update TcError::NulError to also carry the input

I did not change the TcError Shape, this is a separate issue out of scope of this PR. Happy to do a follow up PR.

should be InvalidPath, no?

That comment was on MapError::InvalidName at aya/src/maps/mod.rs:612, which this PR doesn't touch. I agree the variant should either be InvalidPath or carry a #[source] for the dropped NulError. Happy to follow up in another PR.

A couple of additional notes:

This PR also covers aya/src/programs/raw_trace_point.rs:55, which had an unmarked .unwrap() in the same family. I saw this from Fix TODOs of unwrap calls by implementing Error Variants #1389's coverage.
The pre-existing PinError::InvalidPinPath at aya/src/pin.rs still uses error: NulError. Aligning it to source is a breaking pattern-match change unrelated to the TODO cleanup, so I left it for a follow-up. Let me know if you'd prefer it bundled.

tamird

That comment was on MapError::InvalidName at aya/src/maps/mod.rs:612, which this PR doesn't touch. I agree the variant should either be InvalidPath or carry a #[source] for the dropped NulError. Happy to follow up in another PR.

The pre-existing PinError::InvalidPinPath at aya/src/pin.rs still uses error: NulError. Aligning it to source is a breaking pattern-match change unrelated to the TODO cleanup, so I left it for a follow-up. Let me know if you'd prefer it bundled.

I would prefer these changes in this PR but in separate commits. Thanks for your contribution!

@tamird reviewed 9 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on surno).

aya/src/programs/tc.rs line 315 at r1 (raw file):

I did not change the TcError Shape, this is a separate issue out of scope of this PR. Happy to do a follow up PR.

Would you mind making this a second commit in this PR? I think it's useful to look at the API shape all at once.

aya/src/programs/xdp.rs line 108 at r1 (raw file):
unknown interface seems misleading

It puts the NulError into the existing UnknownInterface since a nul-containing wouldn't exist anyways.

That's true, but perhaps misleading. IMO it'd be better to add a variant.

surno

@surno made 2 comments.
Reviewable status: 3 of 10 files reviewed, 2 unresolved discussions (waiting on tamird).

aya/src/programs/tc.rs line 315 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

I did not change the TcError Shape, this is a separate issue out of scope of this PR. Happy to do a follow up PR.

Would you mind making this a second commit in this PR? I think it's useful to look at the API shape all at once.

Done with two commits. On MapError::InvalidName, the input there is the map name, not a path, so I kept the variant InvalidName and added the source: NulError field instead. Please let me know if you'd like it renamed.

aya/src/programs/xdp.rs line 108 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

unknown interface seems misleading

It puts the NulError into the existing UnknownInterface since a nul-containing wouldn't exist anyways.

That's true, but perhaps misleading. IMO it'd be better to add a variant.

Updated with a separate invariant, ProgramError::InvalidInterfaceName to signal to callers that the name itself was incorrect.

tamird

@codex review

@tamird reviewed 7 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on surno).

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0dea8b8210

ℹ️ 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".

chatgpt-codex-connector · 2026-05-06T10:18:48Z

        #[source]
        /// The source error.
-        error: std::ffi::NulError,
+        source: std::ffi::NulError,


Restore PinError::InvalidPinPath error field name

Renaming PinError::InvalidPinPath from error to source is a public API break for downstream exhaustive pattern matches and struct-variant field access. The commit message explicitly says this rename was intentionally avoided, so the code now contradicts stated intent and semver notes.

Useful? React with 👍 / 👎.

Probably should update the commit message.

Thanks for pointing this out. Turns out it was because the PR didn't match the commit. I didn't updated the line in the PR which claimed it was avoided while the commits on the stack did update it after initial review.
.

Copilot

Pull request overview

This PR eliminates several CString::new(...).unwrap() panic sites across aya by mapping NulError into typed error variants, so invalid inputs (interior NUL bytes) are reported as normal errors instead of crashing callers.

Changes:

Introduces new error variants to surface invalid pin paths, tracepoint names, and interface names as typed errors (instead of panics).
Reshapes TcError::NulError into a struct variant that includes the rejected TC name and the underlying NulError.
Updates API snapshots to reflect the resulting public API changes.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
xtask/public-api/aya.txt	Updates the public API snapshot to include the new/reshaped error variants and fields.
aya/src/programs/xdp.rs	Converts interface name `CString` creation from `unwrap()` to `ProgramError::InvalidInterfaceName`.
aya/src/programs/tc.rs	Reworks TC NUL handling to return `TcError::NulError { name, source }` instead of panicking.
aya/src/programs/raw_trace_point.rs	Converts tracepoint name `CString` creation from `unwrap()` to `ProgramError::InvalidTracepointName`.
aya/src/programs/mod.rs	Adds `ProgramError::InvalidPinPath` and uses it in program loading-from-pin path conversion.
aya/src/programs/links.rs	Adds `LinkError::InvalidPinPath` and uses it in `PinnedLink::from_pin` path conversion.
aya/src/programs/info.rs	Uses `ProgramError::InvalidPinPath` when loading program info from a pinned path.
aya/src/pin.rs	Renames `PinError::InvalidPinPath` field to `source` and marks it as `#[source]`.
aya/src/maps/mod.rs	Adds `MapError::InvalidPinPath`, augments `MapError::InvalidName` with a `source`, and updates path/name conversions.
aya/src/maps/info.rs	Uses `MapError::InvalidPinPath` when loading map info from a pinned path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

surno · 2026-05-06T21:54:26Z

    /// The path for the BPF object is not valid.
    #[error("invalid pin path `{}`", path.display())]
    InvalidPinPath {
        /// The path.
        path: std::path::PathBuf,
-
        #[source]
        /// The source error.
-        error: std::ffi::NulError,
+        source: std::ffi::NulError,
    },


Done. See comment above.

surno · 2026-05-06T21:54:27Z

    /// Invalid map name encountered
    #[error("invalid map name `{name}`")]
    InvalidName {
        /// The map name
        name: String,
+        #[source]
+        /// The source error.
+        source: std::ffi::NulError,
    },


tamird · 2026-05-06T14:01:48Z

        // try to open map in case it's already pinned
        let path = path.as_ref();
        let path_string = match CString::new(path.as_os_str().as_bytes()) {
            Ok(path) => path,
-            Err(error) => {
+            Err(source) => {
                return Err(MapError::PinError {
                    name: Some(name.into()),
                    error: PinError::InvalidPinPath {
                        path: path.to_path_buf(),
-                        error,
+                        source,
                    },
                });


This comment seem worth addressing.

Addressed.

MapData::from_pin and MapData::create_pinned_by_name now return a MapError::InvalidPinPath { path, soruce }

tamird · 2026-05-06T14:01:48Z

-                    error,
+                    source,
                },
            })?;


Seems legit.

Addressed.

MapData::from_pin and MapData::create_pinned_by_name now return a MapError::InvalidPinPath { path, soruce }

tamird

@tamird made 3 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on alessandrod and surno).

tamird · 2026-05-06T14:01:47Z

        #[source]
        /// The source error.
-        error: std::ffi::NulError,
+        source: std::ffi::NulError,


Probably should update the commit message.

tamird · 2026-05-06T14:01:48Z

        // try to open map in case it's already pinned
        let path = path.as_ref();
        let path_string = match CString::new(path.as_os_str().as_bytes()) {
            Ok(path) => path,
-            Err(error) => {
+            Err(source) => {
                return Err(MapError::PinError {
                    name: Some(name.into()),
                    error: PinError::InvalidPinPath {
                        path: path.to_path_buf(),
-                        error,
+                        source,
                    },
                });


This comment seem worth addressing.

tamird · 2026-05-06T14:01:48Z

-                    error,
+                    source,
                },
            })?;


Seems legit.

Five call sites converted user-provided byte sequences into a CString via `.unwrap()`, which would panic if the input contained an interior nul byte. Replace each with proper error returns. Pattern A — path → CString (4 sites): - `ProgramData::from_pinned_path`, `ProgramInfo::from_pin`, `PinnedLink::from_pin`, and `MapInfo::from_pin` now surface a new `InvalidPinPath { path, source }` variant on `ProgramError`, `LinkError`, and `MapError` respectively. The variants mirror the existing `PinError::InvalidPinPath` used on the pin (write) side. - `MapData::create_pinned_by_name` and `MapData::from_pin` previously surfaced the same condition wrapped as `MapError::PinError { error: PinError::InvalidPinPath { .. }, .. }`. They now return `MapError::InvalidPinPath` directly so the same invalid-path condition surfaces under the same variant from every call site. Pattern B — name → CString (1 site): - `RawTracePoint::attach` now returns a new `ProgramError::InvalidTracepointName { name, source }` variant instead of unwrapping. This site had an unmarked `.unwrap()` not flagged by a `// TODO`, but is in the same family as the path conversions and was picked up from aya-rs#1389's coverage. The new variants are not `#[non_exhaustive]`, matching the surrounding enums. Per Cargo's SemVer guide this is a major change for downstream callers that exhaustively `match` on these errors; consistent with how variants are added elsewhere in this repo.

- In TcError, the NulError variant has been expanded to include the offending TC program name which contains an unexpected nul byte with the source error. - In SchedClassifier::do_attach and SchedClassifier::qdisc_detach_program, adds the name of the offending TC name in the new NulError variant. Per Cargo's SemVer guide this is a major change for callers that destructure or construct TcError::NulError (tuple → struct variant); consistent with how variants are reshaped elsewhere in this repo.

surno

@surno made 5 comments and resolved 2 discussions.
Reviewable status: 3 of 10 files reviewed, 3 unresolved discussions (waiting on alessandrod and tamird).

surno · 2026-05-06T21:54:26Z

        #[source]
        /// The source error.
-        error: std::ffi::NulError,
+        source: std::ffi::NulError,


Thanks for pointing this out. Turns out it was because the PR didn't match the commit. I didn't updated the line in the PR which claimed it was avoided while the commits on the stack did update it after initial review.
.

surno · 2026-05-06T21:54:26Z

    /// The path for the BPF object is not valid.
    #[error("invalid pin path `{}`", path.display())]
    InvalidPinPath {
        /// The path.
        path: std::path::PathBuf,
-
        #[source]
        /// The source error.
-        error: std::ffi::NulError,
+        source: std::ffi::NulError,
    },


Done. See comment above.

surno · 2026-05-06T21:54:27Z

    /// Invalid map name encountered
    #[error("invalid map name `{name}`")]
    InvalidName {
        /// The map name
        name: String,
+        #[source]
+        /// The source error.
+        source: std::ffi::NulError,
    },


surno · 2026-05-06T21:54:27Z

        // try to open map in case it's already pinned
        let path = path.as_ref();
        let path_string = match CString::new(path.as_os_str().as_bytes()) {
            Ok(path) => path,
-            Err(error) => {
+            Err(source) => {
                return Err(MapError::PinError {
                    name: Some(name.into()),
                    error: PinError::InvalidPinPath {
                        path: path.to_path_buf(),
-                        error,
+                        source,
                    },
                });


Addressed.

MapData::from_pin and MapData::create_pinned_by_name now return a MapError::InvalidPinPath { path, soruce }

surno · 2026-05-06T21:54:27Z

-                    error,
+                    source,
                },
            })?;


Addressed.

MapData::from_pin and MapData::create_pinned_by_name now return a MapError::InvalidPinPath { path, soruce }

tamird

@codex review

@tamird reviewed 7 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on alessandrod).

chatgpt-codex-connector · 2026-05-07T18:10:53Z

Codex Review: Didn't find any major issues. Delightful!

ℹ️ 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".

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

tamird · 2026-05-08T10:30:06Z

    /// The program is not attached.
    #[error("the program name `{name}` is invalid")]
    InvalidName {
        /// program name
        name: String,
    },


Looks right but preexisting. Would be good to fix in another commit or PR.

+    /// If the given `interface` name is malformed
+    /// [`ProgramError::InvalidInterfaceName`] is returned.


- Adds a new `ProgramError::InvalidInterfaceName` error variant to report when name is malformed with unexpected nul byte sequence - removes unwrap used for reporting nul byte error on the interface name Per Cargo's SemVer guide adding a variant to a non-#[non_exhaustive] enum is a major change for callers that exhaustively match on ProgramError; consistent with how variants are added elsewhere in this repo.

- Aligns with the source field name used by the new variants in "aya: return errors instead of panicking on invalid CString inputs" and with the conventional thiserror naming. This is a SemVer-major change for callers that destructure PinError::InvalidPinPath { error, .. }.

- The CString::new failure at MapData::create previously discarded the underlying NulError. Marking it as #[source] so error reporters can walk the chain. Per Cargo's SemVer guide this is a major change for callers that destructure MapError::InvalidName { name } (now requires { name, source } or { name, .. }); consistent with how struct variants are extended elsewhere in this repo.

surno

@surno resolved 2 discussions.
Reviewable status: 4 of 10 files reviewed, all discussions resolved (waiting on alessandrod and tamird).

tamird · 2026-05-20T00:49:08Z

Apologies for the latency; this needs a rebase now.

surno requested a review from a team as a code owner May 5, 2026 20:04

surno force-pushed the aya/error-variants-for-cstring-unwraps branch 3 times, most recently from 120f10b to 1ec80f7 Compare May 5, 2026 20:35

tamird reviewed May 5, 2026

View reviewed changes

surno force-pushed the aya/error-variants-for-cstring-unwraps branch from 1ec80f7 to 953e8bf Compare May 6, 2026 01:17

surno marked this pull request as draft May 6, 2026 01:26

surno marked this pull request as ready for review May 6, 2026 01:56

surno commented May 6, 2026

View reviewed changes

tamird approved these changes May 6, 2026

View reviewed changes

tamird requested review from alessandrod and Copilot May 6, 2026 10:16

Copilot started reviewing on behalf of tamird May 6, 2026 10:17 View session

chatgpt-codex-connector Bot reviewed May 6, 2026

View reviewed changes

Copilot AI reviewed May 6, 2026

View reviewed changes

tamird reviewed May 6, 2026

View reviewed changes

surno added 2 commits May 6, 2026 13:56

surno force-pushed the aya/error-variants-for-cstring-unwraps branch from 0dea8b8 to 1607287 Compare May 6, 2026 21:49

surno marked this pull request as draft May 6, 2026 21:49

surno commented May 6, 2026

View reviewed changes

surno marked this pull request as ready for review May 6, 2026 22:30

tamird approved these changes May 7, 2026

View reviewed changes

tamird requested a review from Copilot May 7, 2026 18:08

Copilot started reviewing on behalf of tamird May 7, 2026 18:09 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

surno added 3 commits May 7, 2026 20:07

surno force-pushed the aya/error-variants-for-cstring-unwraps branch from 1607287 to 3672dc8 Compare May 8, 2026 03:07

surno commented May 8, 2026

View reviewed changes

		/// If the given `interface` name is malformed
		/// [`ProgramError::InvalidInterfaceName`] is returned.

Conversation

surno commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Approach

SemVer notes

Tests

Test plan

Uh oh!

netlify Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for aya-rs-docs ready!

Uh oh!

tamird commented May 5, 2026

Uh oh!

surno commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

surno left a comment

Choose a reason for hiding this comment

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamird left a comment

Choose a reason for hiding this comment

Uh oh!

surno commented May 5, 2026 •

edited

Loading

netlify Bot commented May 5, 2026 •

edited

Loading

surno commented May 5, 2026 •

edited

Loading