Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
trcrsired wants to merge1 commit intollvm:main
base:main
Choose a base branch
Loading
fromtrcrsired:issue149103

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

@github-actionsGitHub Actions
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 theLLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on theLLVM Discord or on theforums.

@llvmbot
Copy link
Member

@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.incindex 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.cppindex 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.hindex 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: Theioctl constants were not what the kernel expects because thestruct 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 wrongioctl 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: Theioctl constants were not what the kernel expects because thestruct 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 wrongioctl 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.

@trcrsired
Copy link
Author

trcrsired commentedJul 17, 2025•
edited
Loading

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 commentedJul 17, 2025•
edited
Loading

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?

@trcrsiredtrcrsired marked this pull request as draftJuly 17, 2025 04:38
@trcrsiredtrcrsired marked this pull request as ready for reviewJuly 17, 2025 04:48
#defineSANITIZER_TCGETS0x402c7413
#defineSANITIZER_TCSETS0x802c7414
#defineSANITIZER_TCSETSF0x802c7415
#defineSANITIZER_TCSETSW0x802c7416
Copy link
Contributor

@thurstondthurstondJul 17, 2025•
edited
Loading

Choose a reason for hiding this comment

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

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 withSANITIZER_LINUX && SANITIZER_GLIBC.

Copy link
Author

Choose a reason for hiding this comment

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

but you said we need to support PPC to make bots happy

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, you're right.

Guard the deletions with
SANITIZER_LINUX && SANITIZER_GLIBC && !SANITIZER_PPC
?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be hardcodingSANITIZER_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.

Copy link
Author

Choose a reason for hiding this comment

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

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

@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?

#endif
#else
#defineSANITIZER_TERMIOS_IOCTL_CONSTANTS1
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

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

?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

@trcrsiredtrcrsiredJul 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

edit: Hmm, good point, you're right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this conditional? Theioctl interceptors never worked. I don't think it's necessary to keep pretending otherwise for old glibc versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this conditional? Theioctl interceptors 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that unrecognizedioctl 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 theseioctl operations don't have pointers in theioctl data, so all you get is a check on the validity of thestruct termios structure itself. (There are otherioctls that can accept arrays of data via the argument structure, and those cases would be much more interesting to check.)

Copy link
Contributor

@thurstondthurstondJul 18, 2025•
edited
Loading

Choose a reason for hiding this comment

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

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; // Uninitializedioctl(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.)

Copy link
Contributor

@thurstondthurstond 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 :-)

@github-actionsGitHub Actions
Copy link

github-actionsbot commentedJul 18, 2025•
edited
Loading

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

trcrsired commentedJul 18, 2025•
edited
Loading

@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 commentedJul 18, 2025•
edited
Loading

@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 releasehttps://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.

thurstond reacted with thumbs up emoji

@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?

@trcrsiredtrcrsired changed the title[sanitizer] Remove usage of termios ioctl constants on Linux[sanitizer] Remove usage of termios ioctl constants on Linux glibc since 2.41Jul 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@thurstondthurstondthurstond approved these changes

@fweimer-rhfweimer-rhfweimer-rh left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
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
@trcrsired@llvmbot@thurstond@fweimer-rh@MaskRay@jakeegan
[8]ページ先頭
©2009-2025 Movatter.jp