Skip to content

feat: Added bounds check for SkBuff::load_bytes and relevant integration test.#1539

Open
benaryorg wants to merge 2 commits into
aya-rs:mainfrom
benaryorg:pull-1218-redo
Open

feat: Added bounds check for SkBuff::load_bytes and relevant integration test.#1539
benaryorg wants to merge 2 commits into
aya-rs:mainfrom
benaryorg:pull-1218-redo

Conversation

@benaryorg
Copy link
Copy Markdown

@benaryorg benaryorg commented Apr 19, 2026

This is a reboot of #1218, with some minor commit juggling (i.e. moving changes from one commit to the other to make them a little more atomic).

[…], attempting to resolve the invalid zero-sized read error found on discord and in my personal projects.

This PR contains:

The correct inclusive bounds check.

An integration test which fails before the update and succeeds after.

Added/updated tests?

We strongly encourage you to add a test for your changes.

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

Checklist

  • Rust code has been formatted with cargo +nightly fmt.
  • All clippy lints have been fixed.
    You can find failing lints with cargo xtask clippy.
  • Unit tests are passing locally with cargo test.
  • The Integration tests are passing locally.
  • I have blessed any API changes with cargo xtask public-api --bless.

(the last two err out on my machine due to problems in the development environment, not the changes themselves)

Unit test failures:

  • maps::tests::test_create_perf_event_array because /sys/devices/system/cpu/possible is not available in the sandbox I run tests in
  • maps::hash_map::per_cpu_hash_map::tests::test_get_not_found: probably the same thing:
assertion failed: `Err(SyscallError(SyscallError { call: "bpf_map_lookup_elem", io_error: Os { code: 2, kind: NotFound, message: "No such file or directory" } }))` does not match `Err(MapError::KeyNotFound)`

(Optional) What GIF best describes this PR or how it makes you feel?

Me trying to run the tests without setting up the full development environment.
Also: me running the tests against the wrong branch.

bonk

This change is Reviewable

@benaryorg benaryorg requested a review from a team as a code owner April 19, 2026 01:06
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 19, 2026

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 561633a
🔍 Latest deploy log https://app.netlify.com/projects/aya-rs-docs/deploys/69e54eea13ee7800088b9731
😎 Deploy Preview https://deploy-preview-1539--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@benaryorg
Copy link
Copy Markdown
Author

Apologies for that.… turns out having two shells on two different machines in the same repo is not the best idea.
The original test did end up tripping clippy due to panic handling (and some other stuff), so I brought it up to speed based on another test.

Since I usually develop in sandboxed environments (forgive me, but I really don't trust other people's build scripts to not write where they shouldn't) a few things on that end failed.
However, unshare --user --net --mount --map-root-user --keep-caps is a pretty suitable replacement for sudo in case of the unit tests, so those all pass now (leaving the OP untouched in that regard since I'm not 100% certain they operate as they should).
The minimum access required (beyond bubblewrap defaults) seems to be /sys/devices/system/cpu/possible and /sys/kernel/btf/vmlinux.
The integration tests on the other hand are less happy about their lack of privileges and continue to fail, so I'm relying on the CI here.

@tamird
Copy link
Copy Markdown
Member

tamird commented Apr 19, 2026

Looks like this has merge conflicts.

Adds a  test program `read_one` that triggers a verifier error when
attempting to read one byte from the SkBuffContext.

Signed-off-by: benaryorg <binary@benary.org>
This resolves the verifier error triggered by the test case added in the previous commit
@benaryorg
Copy link
Copy Markdown
Author

Unfortunate timing with 8edfe49 getting merged in-between. Should be fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants