[MSan] Fix wrong unpoison size in SignalAction

[Camsyn]

MSan should unpoison the parameters of extended signal handlers.
However, MSan unpoisoned the second parameter with the wrong size
sizeof(__sanitizer_sigaction), inconsistent with its real type
siginfo_t.

This commit fixes this issue by correcting the size to
sizeof(__sanitizer_siginfo).

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Kunqiu Chen (Camsyn)

Changes

MSan should unpoison the parameters of extended signal handlers.
However, MSan unpoisoned the second parameter with the wrong size
sizeof(__sanitizer_sigaction), inconsistent with its real type
siginfo_t.

This commit fixes this issue by correcting the size to
sizeof(__sanitizer_siginfo).

---

Full diff: https://github.com/llvm/llvm-project/pull/144071.diff

1 Files Affected:

• (modified) compiler-rt/lib/msan/msan_interceptors.cpp (+1-1)

diff --git a/compiler-rt/lib/msan/msan_interceptors.cpp b/compiler-rt/lib/msan/msan_interceptors.cpp
index 76255cdb742a3..f94d3cb79aa00 100644
--- a/compiler-rt/lib/msan/msan_interceptors.cpp
+++ b/compiler-rt/lib/msan/msan_interceptors.cpp
@@ -1127,7 +1127,7 @@ static void SignalAction(int signo, void *si, void *uc) {
   SignalHandlerScope signal_handler_scope;
   ScopedThreadLocalStateBackup stlsb;
   UnpoisonParam(3);
-  __msan_unpoison(si, sizeof(__sanitizer_sigaction));
+  __msan_unpoison(si, sizeof(__sanitizer_siginfo));
   __msan_unpoison(uc, ucontext_t_sz(uc));
 
   typedef void (*sigaction_cb)(int, void *, void *);

[fmayer]

lgtm, wonder if we can add a test?

[Camsyn]

lgtm, wonder if we can add a test?

We cannot.

Due to sizeof(sigaction) > sizeof(siginfo_t), the original mistake should have caused MSan's FN (as the overly unpoisoning).

I did try to design a test to trigger this FN via user-customized signal stack (siginfo and ucontext are located in the stack).
However, in glibc-2.39, it holds that
$[\mathrm{siginfo}, \mathrm{siginfo + size_{siginfo}}) \subset [\mathrm{siginfo}, \mathrm{siginfo + size_{sigaction}}) \subset [\mathrm{uc}, \mathrm{uc + size_{uc}})$.

Accordingly, we cannot produce a FN as MSAN also unpoison the third parameter by __msan_unpoison(uc, ucontext_t_sz(uc)).

Anyway, the POSIX standard does not specify that the second argument must be contained by the third parameter. Therefore, we still unpoison the si.

[fmayer]

lgtm. as a sidenote, pressing this icon helps bring the change back to the reviewer's attention

[Camsyn]

lgtm. as a sidenote, pressing this icon helps bring the change back to the reviewer's attention

@fmayer That icon seems to require a formal review (approve/request for change) from the reviewer before it appears (because that icon does not appear in my other open PR, where the reviewer also did not give a formal review, but left a comment).

It seems that in this case, I can only ping through @?

Anyway, thank you for the kind reminder.

[fmayer]

@fmayer That icon seems to require a formal review (approve/request for change) from the reviewer before it appears (because that icon does not appear in my other open PR, where the reviewer also did not give a formal review, but left a comment).

It seems that in this case, I can only ping through @?

Anyway, thank you for the kind reminder.

Yeah I think it depends whether they leave the comment from the Review UI, or the main page of the PR. I mainly said this because if you don't click that but it's there, your PR will not show up in the reviewer's https://github.com/pulls/review-requested anymore after they left the first round of comments. If that symbol is not there, pinging through @ seems to be the correct choice.