Skip to content

Fix segfault when emiting warning for empty contracts with custom layout near the end of storage#16682

Merged
cameel merged 2 commits into
developfrom
fix_custom_storage_layout_warning_UB
Jul 9, 2026
Merged

Fix segfault when emiting warning for empty contracts with custom layout near the end of storage#16682
cameel merged 2 commits into
developfrom
fix_custom_storage_layout_warning_UB

Conversation

@matheusaaguiar

@matheusaaguiar matheusaaguiar commented May 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fix #16681.
Fix #16678.
Fix #16784.

@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from 5988dfb to 2f481bc Compare May 7, 2026 06:22
@matheusaaguiar matheusaaguiar requested review from clonker and msooseth May 7, 2026 15:29
msooseth
msooseth previously approved these changes May 7, 2026
View reviewed changes

@msooseth msooseth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I am bad at reviews.

@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from 2f481bc to 263706a Compare May 7, 2026 17:42
matheusaaguiar
matheusaaguiar commented May 7, 2026
View reviewed changes
Comment thread libsolidity/analysis/PostTypeContractLevelChecker.cpp
for (ContractDefinition const* baseContract: ranges::actions::reverse(_contract.annotation().linearizedBaseContracts))
for (VariableDeclaration const* stateVariable: ranges::actions::reverse(baseContract->stateVariables()))
if (stateVariable->referenceLocation() == VariableDeclaration::Location::Unspecified)
for (ContractDefinition const* baseContract: _contract.annotation().linearizedBaseContracts)

@matheusaaguiar matheusaaguiar May 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I realized that linearizedBaseContracts is in order of the most-derived to most-base.
The last storage variable of the most-derived is the one which is closer to the end of storage.

@cameel cameel Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should use the storage layout class for this (the one that calculates offsets). It's too easy to make this mistake. And the assumption that the order of definitions will match the order in storage is true only for now. When we finally introduce the full syntax for storage layouts, it will be possible for these two to be unrelated.

Though that's probably better done as a separate refactor. We need this fix for the upcoming release.

Comment thread libsolidity/analysis/PostTypeContractLevelChecker.cpp
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from 263706a to 842b794 Compare May 7, 2026 18:40
@msooseth msooseth mentioned this pull request May 12, 2026
Closed
@github-actions github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label May 22, 2026
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label May 23, 2026
@github-actions github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 6, 2026
clonker
clonker reviewed Jun 8, 2026
View reviewed changes
Comment thread Changelog.md Outdated
@github-actions github-actions Bot removed the stale The issue/PR was marked as stale because it has been open for too long. label Jun 8, 2026
@github-actions github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 22, 2026
@github-actions github-actions Bot closed this Jun 30, 2026
@cameel cameel removed stale The issue/PR was marked as stale because it has been open for too long. closed-due-inactivity labels Jul 8, 2026
@cameel cameel reopened this Jul 8, 2026
@argotorg argotorg deleted a comment from github-actions Bot Jul 8, 2026
@argotorg argotorg deleted a comment from github-actions Bot Jul 8, 2026
@cameel

cameel commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This should be rebased on the fix PR.

@argotorg argotorg deleted a comment from github-actions Bot Jul 8, 2026
@argotorg argotorg deleted a comment from github-actions Bot Jul 8, 2026
@cameel

cameel commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Fix #16681 and #16678.

Note that this will only close #16681 when the PR is merged. You have to put fix/fixes/closes` next to every issue separately.

@matheusaaguiar matheusaaguiar changed the title Fix UB when emiting warning for empty contracts with custom layout near the end of storage Fix segfault when emiting warning for empty contracts with custom layout near the end of storage Jul 8, 2026
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Jul 8, 2026
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from 05b8cf0 to b69dd6f Compare July 8, 2026 18:57
@matheusaaguiar matheusaaguiar changed the base branch from develop to fix-post-type-contract-level-checker July 8, 2026 19:00
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from b69dd6f to 105f689 Compare July 8, 2026 19:01
@matheusaaguiar matheusaaguiar mentioned this pull request Jul 8, 2026
Merged
1 task
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from 105f689 to deb1c32 Compare July 8, 2026 19:22
Base automatically changed from fix-post-type-contract-level-checker to develop July 8, 2026 20:56
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jul 8, 2026
cameel
cameel reviewed Jul 8, 2026
View reviewed changes
Comment thread Changelog.md
cameel
cameel reviewed Jul 8, 2026
View reviewed changes
Comment thread test/cmdlineTests/storage_layout_specifier_with_inheritance/err
cameel
cameel previously approved these changes Jul 8, 2026
View reviewed changes

@cameel cameel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Needs rebase.

blishko
blishko reviewed Jul 8, 2026
View reviewed changes
Comment thread test/libsolidity/syntaxTests/storageLayoutSpecifier/warning_near_the_storage_end.sol Outdated
Comment thread test/libsolidity/syntaxTests/storageLayoutSpecifier/warning_near_the_storage_end.sol
// ----
// Warning 3495: (11-35): This contract is very close to the end of storage. This limits its future upgradability.
// Warning 3495: (50-74): This contract is very close to the end of storage. This limits its future upgradability.
// Warning 3495: (89-109): This contract is very close to the end of storage. This limits its future upgradability.

@blishko blishko Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't quite understand what is going on in this example. Why don't you get the secondary note about the last state variable here?

@blishko blishko Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And why are you rewriting this test? Was there a problem with it? Shouldn't this be a new test rather than rewriting an existing one?

@cameel cameel Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Syntax tests do not print secondary notes. This is usually what we want because they are very verbose. This does mean that we need a command-line test if we want to test them. Maybe we should add a new test setting (off by default) that forces them to show up when we do want them?

As for a new test, I think it's fine either way. I'd probably lean towards having a separate case for this as well, but it's also just a minor variation so seems acceptable to me to have one test covering both.

@matheusaaguiar matheusaaguiar Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just rewrote it to cover these similar variations altogether as cameel mentioned.

cameel
cameel reviewed Jul 8, 2026
View reviewed changes
Comment thread test/cmdlineTests/storage_layout_specifier_with_inheritance/err
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from deb1c32 to ed1686c Compare July 9, 2026 00:44
@matheusaaguiar matheusaaguiar force-pushed the fix_custom_storage_layout_warning_UB branch from ed1686c to 6c92d0b Compare July 9, 2026 02:42
@cameel cameel merged commit 6fa8151 into develop Jul 9, 2026
83 checks passed
@cameel cameel deleted the fix_custom_storage_layout_warning_UB branch July 9, 2026 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clonker clonker clonker left review comments
@blishko blishko blishko left review comments
@cameel cameel cameel approved these changes
@msooseth msooseth msooseth left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

5 participants

@matheusaaguiar @cameel @clonker @blishko @msooseth