[sanitizer] Remove usage of termios ioctl constants on Linux #149140
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: cqwrteur (trcrsired) Changesglibc 2.42 made all usage of termios ioctl constants strictly internal Therefore, we remove all usage for those removed constants. This should only apply for Linux. Fix #149103 Reference: @fweimer-rh @tstellar Full diff: https://github.com/llvm/llvm-project/pull/149140.diff 3 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc
index 08c2be47f5358..da56214eae1a5 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc
@@ -344,12 +344,8 @@ static void ioctl_table_fill() {
_(SOUND_PCM_WRITE_CHANNELS, WRITE, sizeof(int));
_(SOUND_PCM_WRITE_FILTER, WRITE, sizeof(int));
_(TCFLSH, NONE, 0);
- _(TCGETS, WRITE, struct_termios_sz);
_(TCSBRK, NONE, 0);
_(TCSBRKP, NONE, 0);
- _(TCSETS, READ, struct_termios_sz);
- _(TCSETSF, READ, struct_termios_sz);
- _(TCSETSW, READ, struct_termios_sz);
_(TCXONC, NONE, 0);
_(TIOCGLCKTRMIOS, WRITE, struct_termios_sz);
_(TIOCGSOFTCAR, WRITE, sizeof(int));
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
index 7a89bf1c74985..282148d6b7001 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp
@@ -780,15 +780,11 @@ unsigned struct_ElfW_Phdr_sz = sizeof(Elf_Phdr);
#endif // SOUND_VERSION
unsigned IOCTL_TCFLSH = TCFLSH;
unsigned IOCTL_TCGETA = TCGETA;
- unsigned IOCTL_TCGETS = TCGETS;
unsigned IOCTL_TCSBRK = TCSBRK;
unsigned IOCTL_TCSBRKP = TCSBRKP;
unsigned IOCTL_TCSETA = TCSETA;
unsigned IOCTL_TCSETAF = TCSETAF;
unsigned IOCTL_TCSETAW = TCSETAW;
- unsigned IOCTL_TCSETS = TCSETS;
- unsigned IOCTL_TCSETSF = TCSETSF;
- unsigned IOCTL_TCSETSW = TCSETSW;
unsigned IOCTL_TCXONC = TCXONC;
unsigned IOCTL_TIOCGLCKTRMIOS = TIOCGLCKTRMIOS;
unsigned IOCTL_TIOCGSOFTCAR = TIOCGSOFTCAR;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
index a2b6c37d5450c..8ed4143d3c623 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -1313,15 +1313,11 @@ extern unsigned IOCTL_SNDCTL_COPR_WCODE;
extern unsigned IOCTL_SNDCTL_COPR_WDATA;
extern unsigned IOCTL_TCFLSH;
extern unsigned IOCTL_TCGETA;
-extern unsigned IOCTL_TCGETS;
extern unsigned IOCTL_TCSBRK;
extern unsigned IOCTL_TCSBRKP;
extern unsigned IOCTL_TCSETA;
extern unsigned IOCTL_TCSETAF;
extern unsigned IOCTL_TCSETAW;
-extern unsigned IOCTL_TCSETS;
-extern unsigned IOCTL_TCSETSF;
-extern unsigned IOCTL_TCSETSW;
extern unsigned IOCTL_TCXONC;
extern unsigned IOCTL_TIOCGLCKTRMIOS;
extern unsigned IOCTL_TIOCGSOFTCAR;
|
|
Won't this change cause a regression (failing to intercept and sanitize those ioctls) on all pre-2.42 glibc? Also, those ioctls are listed under SANITIZER_LINUX, not SANITIZER_GLIBC, so I suspect they're used by non-glibc as well. |
Yes, but it does not matter: The (There is an exception for POWER, which has userspace emulation in glibc based on glibc's wrong |
I see, thanks for the explanation!
To clarify, does this apply to PowerPC as well, or only IBM POWER? If the former, there are still PowerPC sanitizer bots (e.g., https://lab.llvm.org/buildbot/#/builders/72) and it would be desirable to add |
|
Hi @thurstond @fweimer-rh . I have seen other usages of struct termios. Do we need to remove them too? I am confused. Please show me the pesudo code of one constant on how to deal with them. Guard against glibc or remove them completely or use kernel_xxx instead? |
Please remove as little as possible (only to fix known issues, or if you can been able to test the change against other affected platforms) to avoid potential regressions. For example, @fweimer-rh mentioned that this change could negatively affect POWER; it's a niche case, but let's not break it unnecessarily. There's also other libcs e.g., musl: do we know for sure that the existing termios references are not needed there?
Guarding against glibc would work. Alternatively, maybe something like |
Well the problem to use TCGETS macro is that And I do not know whether glibc on PowerPC still defines TCGETS either, since i think it does not. I have checked the header files an I do not find KERNEL_TCGETS to use either. |
How about: or logically equivalent rewrite? That would make it easy to reason that it will not break MUSL on any platform, it will not break glibc on PowerPC, or any other combination we haven't thought about. |
|
What about this now? |
| # define SANITIZER_TCGETS 0x402c7413 | ||
| # define SANITIZER_TCSETS 0x802c7414 | ||
| # define SANITIZER_TCSETSF 0x802c7415 | ||
| # define SANITIZER_TCSETSW 0x802c7416 |
There was a problem hiding this comment.
My understanding was glibc doesn't want us to use these constants and that there is no need to intercept these ioctls under glibc because it has historically not worked correctly.
So the simplest fix would be your original change (I can't refer to it anymore because of the force-push) that deleted unused lines, but guard the deletions with SANITIZER_LINUX && SANITIZER_GLIBC.
There was a problem hiding this comment.
but you said we need to support PPC to make bots happy
There was a problem hiding this comment.
Sorry, you're right.
Guard the deletions with
SANITIZER_LINUX && SANITIZER_GLIBC && !SANITIZER_PPC
?
There was a problem hiding this comment.
I do not think you can simply guard the deletions with !SANITIZER_PPC since i believe these constants are removed from glibc too for PPC.
There was a problem hiding this comment.
I don't think we should be hardcoding SANITIZER_TCGETS 0x402c7413 etc., because it's not portable. Besides, the aim of the sanitizers is not to expose or replace functionality that glibc does not; we only need to intercept whatever functionality glibc provides.
Let's revisit the #ifdef TCGETS approach. It does add a macro to sanitizer_platform.h, but so does the current patch (SANITIZER_TERMIOS_IOCTL_CONSTANTS), so it's not worse in that respect. It's cleaner that checking for SANITIZER_LINUX etc., because it directly checks the root cause, which is whether TCGETS is exported or not.
There was a problem hiding this comment.
i mean the best approach is to break the ppc bots for this. or Check glibc version to see whether it is smaller than 2.42
|
@thurstond I have changed the patch to only use constants below 2.41, I think this won't break PPC bots right? |
| # endif | ||
| # else | ||
| # define SANITIZER_TERMIOS_IOCTL_CONSTANTS 1 | ||
| # endif |
There was a problem hiding this comment.
Instead of all the SANITIZER_ and __GLIBC_PREREQ conditions, would the following work instead:
# ifdef TCGETS
# define SANITIZER_TERMIOS_IOCTL_CONSTANTS 0
# else
# define SANITIZER_TERMIOS_IOCTL_CONSTANTS 1
# endif
?
There was a problem hiding this comment.
Including those headers would expose macro definitions to the public interface, potentially contaminating it. For any project leveraging sanitizer APIs, this could introduce conflicts or breakage in otherwise valid code.
There was a problem hiding this comment.
One of your earlier proposed commits (d011ca7) had:
# else
# define SANITIZER_TCGETS TCGETS
# define SANITIZER_TCSETS TCSETS
# define SANITIZER_TCSETSF TCSETSF
# define SANITIZER_TCSETSW TCSETSW
# endif
so I don't think the #ifdef TCGETS approach needs to introduce any extra headers?
There was a problem hiding this comment.
That does not mean TCGETS is defined at that point. You can define the macros later on.
Plus it does not include the part of detection for the existence of TCGETS
There was a problem hiding this comment.
edit: Hmm, good point, you're right.
There was a problem hiding this comment.
Why make this conditional? The ioctl interceptors never worked. I don't think it's necessary to keep pretending otherwise for old glibc versions.
There was a problem hiding this comment.
Why make this conditional? The
ioctlinterceptors never worked. I don't think it's necessary to keep pretending otherwise for old glibc versions.
IIUC you mentioned earlier that these ioctl interceptors did work for PowerPC. I'd prefer not to break them.
There was a problem hiding this comment.
I assume that unrecognized ioctl operations are just passed through, so they would keep working. I'm not sure that sanitizer checking on the argument data is very valuable here because these ioctl operations don't have pointers in the ioctl data, so all you get is a check on the validity of the struct termios structure itself. (There are other ioctls that can accept arrays of data via the argument structure, and those cases would be much more interesting to check.)
There was a problem hiding this comment.
The sanitizers need the interceptors to know that the struct has been written to, to avoid false positives in subsequent checks e.g.,
struct termios tio; // Uninitialized
ioctl(fd, TCGETS, &tio);
printf("output baud rate: %u\n", tio.c_ospeed);
// ^ MemorySanitizer would have a false positive here because it doesn't know that the ioctl initialized tio
(since the interceptor is in sanitizer_common, it is also used in AddressSanitizer and ThreadSanitizer. I believe omitting it may result in false positives for ThreadSanitizer as well.)
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
glibc 2.42 made all usage of termios ioctl constants strictly internal. Therefore, we remove all usage for those removed constants for glibc. We keep a pesudo definition for PowerPC to make bots happy. [sanitizer] avoid using ioctl constants for glibc above 2.41 [compiler-rt] code formatting to make CI happy [compiler-rt] fix a missing #endif
|
@thurstond can you make ci run now? if it passes then please merge it. plus also need to cherry pick to update release branches for llvm21, llvm20, llvm 19 |
llvm 19.x no longer gets new minor releases. For llvm 20.x, llvm 20.1.8 was supposed to be the last minor release https://discourse.llvm.org/t/llvm-20-1-8-plans/87207 Hope that @jakeegan can find a tester for powerpc64 Linux. |
|
@MaskRay Confirmed this PR passes on ppc64le-linux. |
glibc 2.42 made all usage of termios ioctl constants strictly internal
Therefore, we remove all usage for those removed constants.
This should only apply for Linux.
Fix #149103
Reference:
bminor/glibc@3d3572f
@fweimer-rh @tstellar