Skip to content

#21 call asan on setjmp#22

Open
balupillai wants to merge 4 commits intomainfrom
lua-21
Open

#21 call asan on setjmp#22
balupillai wants to merge 4 commits intomainfrom
lua-21

Conversation

@balupillai
Copy link
Copy Markdown
Contributor

@balupillai balupillai commented Apr 2, 2026

Note

Low Risk
Change is gated to ASan builds and only affects how non-local jumps are implemented under sanitizer; production behavior remains unchanged, but it touches core error/exception jump plumbing.

Overview
When __SANITIZE_ADDRESS__ is enabled, src/thrlua.h now bypasses the custom arch-specific asm lua_setjmp_*/lua_longjmp_* wrappers and maps lua_do_setjmp/lua_do_longjmp directly to libc setjmp/longjmp.

This allows ASan to intercept jumps and correctly unpoison skipped stack frames during Lua error handling, reducing ASan false positives while keeping the existing asm fast paths for non-ASan builds.

Reviewed by Cursor Bugbot for commit 511acc4. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread src/luaconf.h Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Lua’s longjmp-based error handling to better cooperate with AddressSanitizer by invoking an ASan “no return” hook before performing longjmp.

Changes:

  • Adds a declaration for __asan_handle_no_return.
  • Wraps LUAI_THROW (for _longjmp/_setjmp and longjmp/setjmp configurations) to call __asan_handle_no_return() before lua_do_longjmp.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/luaconf.h Outdated
Comment on lines +695 to +697

void __asan_handle_no_return(void);

Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

__asan_handle_no_return is declared unconditionally, but the symbol is only provided when linking with the ASan runtime. In non-ASan builds this will either cause a link error once referenced, or at minimum introduces an undocumented dependency. Consider wrapping the declaration (and any calls) in an ASan feature check (e.g. __SANITIZE_ADDRESS__ or __has_feature(address_sanitizer)), and define a no-op fallback when ASan is not enabled; alternatively include sanitizer/asan_interface.h conditionally when available.

Copilot uses AI. Check for mistakes.
Comment thread src/luaconf.h
Comment on lines 706 to 708
/* in Unix, try _longjmp/_setjmp (more efficient) */
#define LUAI_THROW(L,c) lua_do_longjmp((c)->b, 1)
#define LUAI_THROW(L,c) do { __asan_handle_no_return(); lua_do_longjmp((c)->b, 1); } while(0)
#define LUAI_TRY(L,c,a) if (lua_do_setjmp((c)->b) == 0) { a }
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

LUAI_THROW now unconditionally calls __asan_handle_no_return(). Without guarding this call behind an AddressSanitizer-enabled compile check (or providing a no-op fallback), non-ASan builds can fail to link or may inadvertently require the ASan runtime. Gate the call behind an __SANITIZE_ADDRESS__/__has_feature(address_sanitizer) check or similar.

Copilot uses AI. Check for mistakes.
Comment thread src/luaconf.h
Comment on lines 712 to 714
/* default handling with long jumps */
#define LUAI_THROW(L,c) lua_do_longjmp((c)->b, 1)
#define LUAI_THROW(L,c) do { __asan_handle_no_return(); lua_do_longjmp((c)->b, 1); } while(0)
#define LUAI_TRY(L,c,a) if (lua_do_setjmp((c)->b) == 0) { a }
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

Same issue as above: this LUAI_THROW variant calls __asan_handle_no_return() unconditionally, which can break non-ASan builds. Please guard it behind an ASan-enabled macro check or provide a no-op fallback when ASan is not active.

Copilot uses AI. Check for mistakes.
@balupillai
Copy link
Copy Markdown
Contributor Author

DON'T DELETE

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 27c45ad. Configure here.

Comment thread src/thrlua.h
*/
#if defined(__SANITIZE_ADDRESS__)
# define lua_do_setjmp setjmp
# define lua_do_longjmp longjmp
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ASAN detection misses Clang before version 21

Low Severity

The ASAN check uses only __SANITIZE_ADDRESS__, which GCC has supported since 4.8 but Clang only added in version 21 (2026). Clang versions before 21 use __has_feature(address_sanitizer) instead. Builds with older Clang and -fsanitize=address will silently skip this block and continue using the custom asm setjmp/longjmp, defeating the purpose of the ASAN fix. The common portable pattern checks both: defined(__SANITIZE_ADDRESS__) || (defined(__has_feature) && __has_feature(address_sanitizer)).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 27c45ad. Configure here.

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