Skip to content

[sanitizer] Remove usage of termios ioctl constants on Linux glibc since 2.41 #149140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trcrsired
Copy link

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

Copy link

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 @ followed by their GitHub username.

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.

@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2025

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

Author: cqwrteur (trcrsired)

Changes

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


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

3 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc (-4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.cpp (-4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h (-4)
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;

@thurstond
Copy link
Contributor

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.

@fweimer-rh
Copy link
Contributor

Won't this change cause a regression (failing to intercept and sanitize those ioctls) on all pre-2.42 glibc?

Yes, but it does not matter: The ioctl constants were not what the kernel expects because the struct termios definition is different from the kernel UAPI definition.

(There is an exception for POWER, which has userspace emulation in glibc based on glibc's wrong ioctl constants. I don't think it makes sense to keep this around for POWER's sake.)

@thurstond
Copy link
Contributor

Won't this change cause a regression (failing to intercept and sanitize those ioctls) on all pre-2.42 glibc?

Yes, but it does not matter: The ioctl constants were not what the kernel expects because the struct termios definition is different from the kernel UAPI definition.

I see, thanks for the explanation!

(There is an exception for POWER, which has userspace emulation in glibc based on glibc's wrong ioctl constants. I don't think it makes sense to keep this around for POWER's sake.)

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 #if !SANITIZER_PPC (or related constants such as SANITIZER_PPC64V2 etc.) to prevent a regression.

@vitalybuka vitalybuka requested a review from thurstond July 17, 2025 00:54
@trcrsired
Copy link
Author

trcrsired commented Jul 17, 2025

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?

@thurstond
Copy link
Contributor

I have seen other usages of struct termios. Do we need to remove them too?

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?

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?

Guarding against glibc would work. Alternatively, maybe something like #ifdef TCGETS (plus a comment about glibc).

@trcrsired
Copy link
Author

trcrsired commented Jul 17, 2025

I have seen other usages of struct termios. Do we need to remove them too?

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?

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?

Guarding against glibc would work. Alternatively, maybe something like #ifdef TCGETS (plus a comment about glibc).

Well the problem to use TCGETS macro is that
compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
is not supposed to introduce dependencies or it would cause macro pollution.

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.

@thurstond
Copy link
Contributor

I have seen other usages of struct termios. Do we need to remove them too?

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?

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?

Guarding against glibc would work. Alternatively, maybe something like #ifdef TCGETS (plus a comment about glibc).

Well the problem to use TCGETS macro is that compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h is not supposed to introduce dependencies or it would cause macro pollution.

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:

#if SANITIZER_LINUX
...
#if !SANITIZER_GLIBC
// move the existing TCGETS stuff here
#endif
...
#endif

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.

@trcrsired
Copy link
Author

What about this now?

@trcrsired trcrsired marked this pull request as draft July 17, 2025 04:38
@trcrsired trcrsired marked this pull request as ready for review July 17, 2025 04:48
@trcrsired
Copy link
Author

@thurstond I have changed the patch to only use constants below 2.41, I think this won't break PPC bots right?

Copy link
Contributor

@thurstond thurstond left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, and thanks for bearing through my bad review ideas :-)

Copy link

github-actions bot commented Jul 18, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@trcrsired trcrsired force-pushed the issue149103 branch 2 times, most recently from c7f3bf0 to 5f406ad Compare July 18, 2025 08:34
@trcrsired
Copy link
Author

trcrsired commented Jul 18, 2025

@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

@MaskRay
Copy link
Member

MaskRay commented Jul 18, 2025

@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
However, given the build breakage with new glibc we should consider making another one...

Hope that @jakeegan can find a tester for powerpc64 Linux.

@jakeegan
Copy link
Member

@MaskRay Confirmed this PR passes on ppc64le-linux.

@thurstond
Copy link
Contributor

Thank you @MaskRay for the pointers and @jakeegan for testing on powerpc64!

Does anyone have any remaining concerns for landing this patch?

@trcrsired trcrsired changed the title [sanitizer] Remove usage of termios ioctl constants on Linux [sanitizer] Remove usage of termios ioctl constants on Linux glibc since 2.41 Jul 19, 2025
@trcrsired
Copy link
Author

@MaskRay @thurstond Time to merge the code. Thank you:)

@MaskRay
Copy link
Member

MaskRay commented Jul 19, 2025

This commit will be authored by [email protected].

LLVM wants contributors to use a non-hidden email address. https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223

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
@trcrsired
Copy link
Author

trcrsired commented Jul 20, 2025

This commit will be authored by [email protected].

LLVM wants contributors to use a non-hidden email address. https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223

Hi @MaskRay, I have disabled the hidden email account in the GitHub setting and rebased the commit. Can you have a look?

@trcrsired
Copy link
Author

hi? is anyone here to merge the patch?

@thurstond
Copy link
Contributor

thurstond commented Jul 22, 2025

hi? is anyone here to merge the patch?

The buildbots (http://google.github.io/sanitizers/show_bots.html), especially the ASan and MSan bots, are in a bad state. Out of an abundance of caution, it would be good to wait until they are green again before merging.

@trcrsired
Copy link
Author

hi? is anyone here to merge the patch?

The buildbots (http://google.github.io/sanitizers/show_bots.html), especially the ASan and MSan bots, are in a bad state. Out of an abundance of caution, it would be good to wait until they are green again before merging.

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[compiler-rt]use of undeclared identifier 'TCGETS' for x86_64-linux-gnu (latest linux kernel and glibc)
6 participants