Skip to content

Try to be smarter with our resolve_gep calls to not duplicate work with resolve_check_bounds#71

Open
srosefield-riverside wants to merge 3 commits intomainfrom
feature/smarter_gep
Open

Try to be smarter with our resolve_gep calls to not duplicate work with resolve_check_bounds#71
srosefield-riverside wants to merge 3 commits intomainfrom
feature/smarter_gep

Conversation

@srosefield-riverside
Copy link
Collaborator

@srosefield-riverside srosefield-riverside commented Dec 19, 2025

GEP knows the size of the type the derived pointer points at, so we can just include that bounds check directly in our resolve_gep call. Then we can elide any call to resolve_check_bounds that is based on a resolve_gep output as bounds checking was already completed.

The stack allocation lifetime tracking was updated to use llvm's lifetime markers if they are available. This also helped with an error in optimized builds where stack addresses were re-used but we marked them as freed after the first lifetime ended.

@srosefield-riverside srosefield-riverside changed the title WIP Try to be smarter with our resolve_gep calls to not duplicate work with resolve_check_bounds Try to be smarter with our resolve_gep calls to not duplicate work with resolve_check_bounds Dec 22, 2025
@srosefield-riverside srosefield-riverside marked this pull request as ready for review December 22, 2025 14:55
…th resolve_check_bounds

- Insert stack tracking at more appropriate location if we find llvm lifetime markers
- We only ever invalidate one address at a time, so do that instead of invalidating a range which is more expensive.
- Move stack allocations to their own thread_local so we dont need to acquire a lock.
- Reduce the number of times we call invalidate_stack
- Hint that the resolve_gep branch is likely true.
- coalesce neighboring GEP calls
@rjsmith1999
Copy link
Collaborator

This has collected a couple of different things, I'm still not sure about the gep changes, but we should merge the stack improvements separately? Can you pull out the stack instrumentation using lifetime annotations into its own pr? Maybe also the thread local stack object map?

@srosefield-riverside
Copy link
Collaborator Author

Sure let me know which things you want me to extract. Technically if we just remove this block or add a "strategy != SAFE" in https://github.com/riversideresearch/resolve/pull/71/files#diff-dab9e2f70474350daa0981a1b1db050296682bd2a4f6b1b83c5a4a768d5215d1R723 then we'll also call resolve_check_bounds after the GEPs.

@srosefield-riverside
Copy link
Collaborator Author

Also FWIW this is what the flamegraph on redis looks like with these changes. There is still a substantial amount of locking overhead on resolve_gep because some of the main pointers checked aren't known to resolve and so it goes through the whole function to the fallthrough true case. Of course there is no-longer any locking in the resolve_stack_object and invalidate_stack functions (although there are some atomic compares for the thread locals).

flamegraph

@rjsmith1999
Copy link
Collaborator

Flame graph looks even better! Still seems like a lot of synchronization overhead though.

Why do thread locals have atomic operations in them again? Is it related to initialization?

If you can extract from the logs what the pointers are that are untracked, we can make a plan to track them...

@rjsmith1999
Copy link
Collaborator

Sure let me know which things you want me to extract. Technically if we just remove this block or add a "strategy != SAFE" in https://github.com/riversideresearch/resolve/pull/71/files#diff-dab9e2f70474350daa0981a1b1db050296682bd2a4f6b1b83c5a4a768d5215d1R723 then we'll also call resolve_check_bounds after the GEPs.

I can't tell from the link which block you're referring to?

@srosefield-riverside
Copy link
Collaborator Author

Ah, because the "bounds_check.cpp" is collapsed by default it doesn't jump to the line, which is

if (called && called->getName() == "resolve_gep_sanitized") { return; }

The atomics are indeed for initialization (and destruction). It needs to check if it has been created yet.

@srosefield-riverside
Copy link
Collaborator Author

Flame graph looks even better! Still seems like a lot of synchronization overhead though.

Why do thread locals have atomic operations in them again? Is it related to initialization?

If you can extract from the logs what the pointers are that are untracked, we can make a plan to track them...

Quickly checking some logs, there are millions on lines like

[2026-01-08T14:12:16Z WARN  resolve::remediate] [GEP] Cannot find ptr 0x6529b019e37b in shadow table
...
[2026-01-08T14:13:59Z WARN  resolve::remediate] [GEP] Cannot find ptr 0x73f1b51ed010 in shadow table

and from strace it looks like they are in the address spaces returned by brk and mmap

strace: Process 469010 attached
brk(0x6529bb628000)                     = 0x6529bb628000
brk(0x6529bb654000)                     = 0x6529bb654000
brk(0x6529bb676000)                     = 0x6529bb676000
brk(0x6529bb69a000)                     = 0x6529bb69a000
mmap(NULL, 462848, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x73f1b51ed000

@rjsmith1999
Copy link
Collaborator

So maybe these are heap allocations coming from libc but not malloc? Hmm...

@rjsmith1999
Copy link
Collaborator

rjsmith1999 commented Jan 9, 2026

Sure let me know which things you want me to extract.

Sorry I missed this part of your comment. Basically, just
A) The new instrument alloca's that considers lifetime annotations. ** This one is important for correctness; other ones are just performance tweaks **
B) The thread local stack object list, and relevant libresolve changes. ** a performance tweak, but seems pretty uncontroversial as adds almost no complexity**

I want to hold off on the other performance tweaks until we have a chance to discuss overall strategy for performance

@rjsmith1999 rjsmith1999 added the CVEAssert Related to CVEAssert component label Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CVEAssert Related to CVEAssert component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants