Skip to content
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

handle some malformed 9P requests gracefully #110

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Conversation

garlick
Copy link
Member

@garlick garlick commented Dec 29, 2024

This fixes diod's poor (segfaulting) handling of a couple of malformed requests reported by @rtmrtmrtmrtm.

Copy link
Member

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@grondo
Copy link
Member

grondo commented Dec 30, 2024

Hmm, one comment got dropped. typo 'reqst' in commit message of 7df07c7

Problem: when a read/write to an xattr has invalid arguments, it sets
np_uerror (EINVAL) but the caller then sets np_uerror (errno),
potentially clearing the original error and triggering an assertion
in np_req_respond().

Set errno = EINVAL instead.
Problem: the Twrite xattr offset is not checked, so a malformed request
can trigger a diod server segfault or worse.

Check the offset in Tread and Twrite.

Add a test

Thanks to @rtmrtmrtmrtm for the bug report and reproducer.
Problem: buf_check_size() doesn't flag a buffer overflow if
the buffer is exactly full when the check is performed.

For example, a malformed Tlink with name size > 0 but no name causes
a segfault.

Fix off by one check so this and similar malformed requests
generate an error response.

Add unit test.

Fixes chaos#109
@garlick
Copy link
Member Author

garlick commented Dec 30, 2024

Thanks! Fixed that and rebased. I'll set MWP.

Copy link
Contributor

mergify bot commented Dec 30, 2024

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #110 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (detail: 2 of 2 required status checks are queued.)

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@garlick
Copy link
Member Author

garlick commented Dec 30, 2024

@Mergifyio requeue

Copy link
Contributor

mergify bot commented Dec 30, 2024

requeue

⚠️ Cannot use the command requeue

⚠ The product Merge queue needs to be activated to enable this feature.

@garlick
Copy link
Member Author

garlick commented Dec 30, 2024

@Mergifyio requeue

Copy link
Contributor

mergify bot commented Dec 30, 2024

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mergify mergify bot merged commit 31f7656 into chaos:master Dec 30, 2024
7 checks passed
@garlick garlick deleted the issue#109 branch December 30, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants