Skip to content

[emval] Prevent creating lvalue refs from thin air#24606

Merged
RReverser merged 1 commit intoemscripten-core:mainfrom
RReverser:prevent-lval-fromwiretype
Jun 19, 2025
Merged

[emval] Prevent creating lvalue refs from thin air#24606
RReverser merged 1 commit intoemscripten-core:mainfrom
RReverser:prevent-lval-fromwiretype

Conversation

@RReverser
Copy link
Collaborator

I'm having trouble reproducing this UB in isolation, but I ran into issues with garbage values in test_i64_val while making other innocent-looking changes. I think it might even explain the really weird bug from #24577 (comment) that magically went away after a rebase despite no related changes being made upstream.

Essentially, because I stupidly used T&& in a template, and because it's [waves around] C++, it got inferred as unsigned long long & instead of the desired unsigned long long, so in .as<T>() we were quietly returning an unsigned long long& reference to a temporary unsigned long long value on function stack.

Most of the time it somehow still works and, because it's templated, it's not caught by Clang's "non-const lvalue reference to type" diagnostic, but under unrelated changes and optimisations it can break badly.

I'm having real trouble reproducing this UB in isolation, but I ran into issues with garbage values in test_i64_val while making other innocent-looking changes. I think it might explain the really weird bug from emscripten-core#24577 (comment) that magically went away after a rebase despite no related changes being made.

Essentially, because I stupidly used `T&&` in a template, and because it's [waves around] C++, it got inferred as `unsigned long long &` instead of the desired `unsigned long long`, so in `.as<T>()` we were quietly returning `unsigned long long&` references to a temporary `unsigned long long` value on function stack.

Most of the time it somehow still works and, because it's templated, it's not caught by Clang's "non-const lvalue reference to type" diagnostic, but under unrelated changes and optimisations it can break badly.
@RReverser RReverser requested review from brendandahl and sbc100 June 18, 2025 23:13
@RReverser
Copy link
Collaborator Author

@sbc100 @brendandahl Can someone take a quick look please? Want to unblock weird bugs in that test in -O1.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Rubberstamp lgtm because I can't see I really understand the change, but if the tests pass it seems reasonable.

@brendandahl can review more fully when he returns from vacation.

@RReverser
Copy link
Collaborator Author

can review more fully when he returns from vacation

Ah hm I guess I'll merge it with rubberstamp for now to get unblocked.

I can't see I really understand the change

I tried to explain in the description, but basically after template expansion we allowed something like

int bar();

int& foo() {
  return bar();
}

which, because it's nested inside templates, is not caught by Clang's static analyzer, and it happens to work correctly quite a lot of time (since the value is still on the stack)... until it doesn't.

@RReverser RReverser enabled auto-merge (squash) June 19, 2025 17:04
@RReverser RReverser merged commit d0460ec into emscripten-core:main Jun 19, 2025
30 checks passed
@RReverser RReverser deleted the prevent-lval-fromwiretype branch June 19, 2025 17:28
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.

2 participants