Skip to content

check for overflow on 32bit relocations#1175

Merged
davidlattimore merged 4 commits into
wild-linker:mainfrom
karolzwolak:warn-about-32bit-reloc-overflow
Oct 11, 2025
Merged

check for overflow on 32bit relocations#1175
davidlattimore merged 4 commits into
wild-linker:mainfrom
karolzwolak:warn-about-32bit-reloc-overflow

Conversation

@karolzwolak
Copy link
Copy Markdown
Contributor

@karolzwolak karolzwolak commented Oct 9, 2025

Fixes #1149

@karolzwolak karolzwolak force-pushed the warn-about-32bit-reloc-overflow branch from fd5c87c to a0e6d07 Compare October 9, 2025 19:13
@karolzwolak
Copy link
Copy Markdown
Contributor Author

karolzwolak commented Oct 9, 2025

Don't mind the additional .gitignore changes and .c files and test.sh.
I couldn't reproduce the error in the actual test infrastructure so I temporarily included in in the PR so someone can tell me how to port it to the test infra.

@davidlattimore what test attributes should I add/remove? The test unexpectedly passes when run via cargo. I didn't see any docs about making new tests. However when compiling the same files (minus the _start shenanigan) using test.sh (assumes PWD is project root and wild is compiled) -- everything works as expected, so the linker reports the error.

davidlattimore
davidlattimore reviewed Oct 9, 2025
View reviewed changes
Comment thread wild/tests/sources/relocation-overflow.c Outdated
Comment thread wild/tests/sources/relocation-overflow.c Outdated
Comment thread linker-utils/src/elf.rs Outdated

#[must_use]
pub const fn from_byte_number(n_bytes: usize) -> Self {
if n_bytes == 0 || n_bytes > 8 {
Copy link
Copy Markdown
Member

@davidlattimore davidlattimore Oct 9, 2025

Choose a reason for hiding this comment

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

This > could be a >= right? i.e. if we've got an 8 byte relocation, we don't need a check.

Copy link
Copy Markdown
Contributor Author

@karolzwolak karolzwolak Oct 10, 2025

Choose a reason for hiding this comment

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

The end result should be the same, but I realized more glaring issue -- this code assumes each relocation is a signed integer. The match should also return whether it should be signed or unsigned.

Copy link
Copy Markdown
Member

@davidlattimore davidlattimore Oct 10, 2025

Choose a reason for hiding this comment

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

Does it matter? I'm not sure it does. We do our computation using u64s with wrapping_add, wrapping_sub etc, so it's possible that it might work fine as-is anyway. I'm not 100% sure that it doesn't matter, but perhaps see if you can come up with a concrete example where it breaks?

Copy link
Copy Markdown
Contributor Author

@karolzwolak karolzwolak Oct 10, 2025

Choose a reason for hiding this comment

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

Hm okay I think you're right. I was thinking about 32 bit signed vs unsigned relocations, but I don't even see the distinction in the match. This function was computing wrong values though because my silly mistake with shift op precedence, which I fixed and also added tests for.

@karolzwolak karolzwolak force-pushed the warn-about-32bit-reloc-overflow branch from a0e6d07 to e4608b8 Compare October 10, 2025 15:52
@karolzwolak karolzwolak marked this pull request as ready for review October 10, 2025 15:53
@karolzwolak
Copy link
Copy Markdown
Contributor Author

karolzwolak commented Oct 10, 2025

CI on tumbleweed seems to be failing because of formatting -- maybe it has different version of the tool?

 thread 'tidy::check_sources_format' (6386) panicked at wild/tests/integration_tests.rs:2856:13:
clang-format check failed:
/__w/wild/wild/wild/tests/sources/relocation-overflow-b.c:10:5: error: code should be clang-formatted [-Wclang-format-violations]
void *keep_b_ref(void) { return &reloc_b_to_a; }

@karolzwolak
Copy link
Copy Markdown
Contributor Author

karolzwolak commented Oct 10, 2025

I created #1183 to help debug such weird cases in CI.

@karolzwolak
Copy link
Copy Markdown
Contributor Author

karolzwolak commented Oct 10, 2025 •
edited
Loading

I temporarily added commit to check clang version to test if the version in CI is the same.

EDIT: the version on tumbleweed is 21.1.2, but on other jobs it varies from 14.x to 20.x.

@karolzwolak karolzwolak force-pushed the warn-about-32bit-reloc-overflow branch 2 times, most recently from a4dfcf5 to e15bbe3 Compare October 10, 2025 16:46
@karolzwolak karolzwolak mentioned this pull request Oct 10, 2025
Closed
@karolzwolak karolzwolak force-pushed the warn-about-32bit-reloc-overflow branch from e15bbe3 to ab9702d Compare October 11, 2025 07:48
@davidlattimore davidlattimore merged commit a5df1b6 into wild-linker:main Oct 11, 2025
20 checks passed
@davidlattimore davidlattimore mentioned this pull request Oct 13, 2025
Closed
@karolzwolak karolzwolak mentioned this pull request Oct 15, 2025
Merged
@karolzwolak karolzwolak deleted the warn-about-32bit-reloc-overflow branch November 25, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidlattimore davidlattimore davidlattimore approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wild doesn't report error if a 32 bit relocation overflows

2 participants

@karolzwolak @davidlattimore