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.

Sign up for GitHub

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

Jump to bottom

coredump: use unaligned_read_ne{32,64}() to parse auxv #26927

Merged
merged 1 commit into from
Mar 22, 2023
Merged

coredump: use unaligned_read_ne{32,64}() to parse auxv #26927

merged 1 commit into from
Mar 22, 2023

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Mar 21, 2023 •
edited
Loading

Fixes a bug introduced by 3e4d0f6.
Fixes #26912.

@yuwata yuwata requested a review from keszybz March 21, 2023 23:50
@github-actions github-actions bot added coredump please-review PR is ready for (re-)review by a maintainer labels Mar 21, 2023
@github-advanced-security

This comment was marked as off-topic.

Sign in to view
@yuwata yuwata mentioned this pull request Mar 21, 2023
Closed
poettering
poettering reviewed Mar 22, 2023
View reviewed changes
src/coredump/coredump.c Outdated Show resolved Hide resolved
@poettering poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Mar 22, 2023
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Mar 22, 2023
@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Mar 22, 2023
@keszybz
Copy link
Member

keszybz commented Mar 22, 2023 •
edited
Loading

The change looks good, but the commit message needs to explain what the issue that is being fixes is, and why we didn't see it so far.

Edit: In other words: the issue looks significant, but wouldn't be a problem outside of an instrumented run, because the buffer would be aligned in all realistic cases. Or at least that's what I think would be the case. But we should state this explicitly, and include this in the commit message, so we all agree what is happening and what the impact of the issue is.

Edit2: actually, the address is unaligned, so maybe it would be an issue outside of an instrumented run too. But on amd64, it's just a slowdown. So a crash would be possible only only other architectures?

@keszybz keszybz added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Mar 22, 2023
@yuwata
coredump: use unaligned_read_ne{32,64}() to parse auxv
c9c9961 
Fixes a bug introduced by 3e4d0f6.

The auxv metadata is unaligned, as the length of the prefix
"COREDUMP_PROC_AUXV=" is 19. Hence, parse_auxv{32,64}() may triger
an undefined behavior (or at least cause slow down), which can be
detected when running on an undefined behavior sanitizer.

This also introduces a macro to define `parse_auxv{32,64}()`.

Fixes systemd#26912.
@yuwata
Copy link
Member Author

yuwata commented Mar 22, 2023

@keszybz The commit message is extended. PTAL.

@yuwata yuwata added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Mar 22, 2023
@keszybz
Copy link
Member

keszybz commented Mar 22, 2023

Perfect! Thanks!

@keszybz
Copy link
Member

keszybz commented Mar 22, 2023

The CI passed previously, so let's merge this, since the commit message shouldn't influence the result. (I know, with CI one can never be sure, but let's be optimistic ;) )

@keszybz keszybz added ci-failure-appears-unrelated and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Mar 22, 2023
@keszybz keszybz merged commit 9b032f9 into systemd:main Mar 22, 2023
@yuwata yuwata deleted the coredump-unaligned-read branch March 22, 2023 15:43
@mrc0mmand mrc0mmand mentioned this pull request Mar 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poettering poettering poettering left review comments

@keszybz keszybz keszybz approved these changes

Assignees
No one assigned
Labels
ci-failure-appears-unrelated coredump
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Coredump undefined behavior sanitizer error in CentOS CI
3 participants
@yuwata @keszybz @poettering