Skip to content

Conversation

@fmayer
Copy link
Contributor

@fmayer fmayer commented Mar 27, 2025

No description provided.

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Mar 27, 2025

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

Author: Florian Mayer (fmayer)

Changes

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

4 Files Affected:

  • (modified) compiler-rt/lib/msan/tests/msan_test.cpp (+69)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc (+63)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h (+4)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h (+9)
diff --git a/compiler-rt/lib/msan/tests/msan_test.cpp b/compiler-rt/lib/msan/tests/msan_test.cpp
index a126dd4fdd55e..8b7df85e0ff04 100644
--- a/compiler-rt/lib/msan/tests/msan_test.cpp
+++ b/compiler-rt/lib/msan/tests/msan_test.cpp
@@ -4908,5 +4908,74 @@ TEST(MemorySanitizer, timer_create) {
   EXPECT_POISONED(timer2);
   timer_delete(timer);
 }
+
+TEST(MemorySanitizer, getservent_r) {
+  struct servent result_buf;
+  struct servent *result;
+  char buf[1024];
+  EXPECT_POISONED(result_buf);
+  EXPECT_POISONED(result);
+  EXPECT_POISONED(buf);
+  ASSERT_EQ(getservent_r(&result_buf, buf, sizeof(buf), &result), 0);
+  EXPECT_NOT_POISONED(result);
+  ASSERT_NE(result, nullptr);
+  EXPECT_NOT_POISONED(result_buf);
+  EXPECT_NOT_POISONED(buf);
+}
+
+TEST(MemorySanitizer, getservbyname_r) {
+  struct servent result_buf;
+  struct servent *result;
+  char buf[1024];
+  EXPECT_POISONED(result_buf);
+  EXPECT_POISONED(result);
+  EXPECT_POISONED(buf);
+  ASSERT_EQ(
+      getservbyname_r("ssh", nullptr, &result_buf, buf, sizeof(buf), &result),
+      0);
+  EXPECT_NOT_POISONED(result);
+  // If this fails, check /etc/services if "ssh" exists. I picked this because
+  // it should exist everywhere, if it doesn't, I am sorry. Disable the test
+  // then please.
+  ASSERT_NE(result, nullptr);
+  EXPECT_NOT_POISONED(result_buf);
+  EXPECT_NOT_POISONED(buf);
+}
+
+TEST(MemorySanitizer, getservbyname_r_unknown) {
+  struct servent result_buf;
+  struct servent *result;
+  char buf[1024];
+  EXPECT_POISONED(result_buf);
+  EXPECT_POISONED(result);
+  EXPECT_POISONED(buf);
+  ASSERT_EQ(getservbyname_r("invalidhadfuiasdhi", nullptr, &result_buf, buf,
+                            sizeof(buf), &result),
+            0);
+  EXPECT_NOT_POISONED(result);
+  ASSERT_EQ(result, nullptr);
+  EXPECT_POISONED(result_buf);
+  EXPECT_POISONED(buf);
+}
+
+TEST(MemorySanitizer, getservbyport_r) {
+  struct servent result_buf;
+  struct servent *result;
+  char buf[1024];
+  EXPECT_POISONED(result_buf);
+  EXPECT_POISONED(result);
+  EXPECT_POISONED(buf);
+  ASSERT_EQ(getservbyport_r(htons(22), nullptr, &result_buf, buf, sizeof(buf),
+                            &result),
+            0);
+  EXPECT_NOT_POISONED(result);
+  // If this fails, check /etc/services if "ssh" exists. I picked this because
+  // it should exist everywhere, if it doesn't, I am sorry. Disable the test
+  // then please.
+  ASSERT_NE(result, nullptr);
+  EXPECT_NOT_POISONED(result_buf);
+  EXPECT_NOT_POISONED(buf);
+}
+
 #endif
 } // namespace
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
index 761dbd3f5a679..1be8a901ab06e 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
@@ -10279,6 +10279,66 @@ INTERCEPTOR(SSIZE_T, freadlink, int fd, char *buf, SIZE_T bufsiz) {
 #  define INIT_FREADLINK
 #endif
 
+UNUSED static void HandleGetServentReentrantResult(
+    void *ctx, int res, struct __sanitizer_servent *result_buf, char *buf,
+    SIZE_T buflen, struct __sanitizer_servent **result) {
+  if (res)
+    return;
+  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, (char *)result, sizeof(void *));
+  if (*result) {
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, (char *)*result,
+                                   sizeof(__sanitizer_servent));
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
+  }
+}
+
+#if SANITIZER_INTERCEPT_GETSERVENT_R
+INTERCEPTOR(int, getservent_r, struct __sanitizer_servent *result_buf,
+            char *buf, SIZE_T buflen, struct __sanitizer_servent **result) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, getservent_r, result_buf, buf, buflen, result);
+  int res = REAL(getservent_r)(result_buf, buf, buflen, result);
+  HandleGetServentReentrantResult(ctx, res, result_buf, buf, buflen, result);
+  return res;
+}
+#  define INIT_GETSERVENT_R COMMON_INTERCEPT_FUNCTION(getservent_r)
+#else
+#  define INIT_GETSERVENT_R
+#endif
+
+#if SANITIZER_INTERCEPT_GETSERVBYNAME_R
+INTERCEPTOR(int, getservbyname_r, const char *name, const char *proto,
+            struct __sanitizer_servent *result_buf, char *buf, SIZE_T buflen,
+            struct __sanitizer_servent **result) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, getservbyname_r, name, proto, result_buf, buf,
+                           buflen, result);
+  COMMON_INTERCEPTOR_READ_STRING(ctx, name, internal_strlen(name));
+  int res = REAL(getservbyname_r)(name, proto, result_buf, buf, buflen, result);
+  HandleGetServentReentrantResult(ctx, res, result_buf, buf, buflen, result);
+  return res;
+}
+#  define INIT_GETSERVBYNAME_R COMMON_INTERCEPT_FUNCTION(getservbyname_r)
+#else
+#  define INIT_GETSERVBYNAME_R
+#endif
+
+#if SANITIZER_INTERCEPT_GETSERVBYPORT_R
+INTERCEPTOR(int, getservbyport_r, int port, const char *proto,
+            struct __sanitizer_servent *result_buf, char *buf, SIZE_T buflen,
+            struct __sanitizer_servent **result) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, getservbyport_r, port, proto, result_buf, buf,
+                           buflen, result);
+  int res = REAL(getservbyport_r)(port, proto, result_buf, buf, buflen, result);
+  HandleGetServentReentrantResult(ctx, res, result_buf, buf, buflen, result);
+  return res;
+}
+#  define INIT_GETSERVBYPORT_R COMMON_INTERCEPT_FUNCTION(getservbyport_r)
+#else
+#  define INIT_GETSERVBYPORT_R
+#endif
+
 #include "sanitizer_common_interceptors_netbsd_compat.inc"
 
 namespace __sanitizer {
@@ -10604,4 +10664,7 @@ static void InitializeCommonInterceptors() {
   INIT_FREADLINK;
 
   INIT___PRINTF_CHK;
+  INIT_GETSERVENT_R;
+  INIT_GETSERVBYNAME_R;
+  INIT_GETSERVBYPORT_R;
 }
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h b/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
index 468b5494d0092..b8f2f738e7478 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h
@@ -645,6 +645,10 @@ SANITIZER_WEAK_IMPORT void *aligned_alloc(__sanitizer::usize __alignment,
 #  define SI_MAC_OS_DEPLOYMENT_MIN_13_00 0
 #endif
 #define SANITIZER_INTERCEPT_FREADLINK (SI_MAC && SI_MAC_OS_DEPLOYMENT_MIN_13_00)
+#define SANITIZER_INTERCEPT_GETSERVENT_R SI_GLIBC
+#define SANITIZER_INTERCEPT_GETSERVBYNAME_R SI_GLIBC
+#define SANITIZER_INTERCEPT_GETSERVBYPORT_R SI_GLIBC
+
 // This macro gives a way for downstream users to override the above
 // interceptor macros irrespective of the platform they are on. They have
 // to do two things:
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 67f00ff6f9e72..bc46c10ca54ce 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_posix.h
@@ -1509,6 +1509,15 @@ extern unsigned IOCTL_KIOCSOUND;
 extern unsigned IOCTL_PIO_SCRNMAP;
 #endif
 
+#  if SANITIZER_GLIBC
+struct __sanitizer_servent {
+  char *s_name;     /* official service name */
+  char **s_aliases; /* alias list */
+  int s_port;       /* port number */
+  char *s_proto;    /* protocol to use */
+};
+#  endif
+
 extern const int si_SEGV_MAPERR;
 extern const int si_SEGV_ACCERR;
 }  // namespace __sanitizer

Created using spr 1.3.4
@fmayer fmayer requested a review from thurstond March 27, 2025 23:14
@thurstond
Copy link
Contributor

The man page says

       getservent_r(), getservbyname_r(), getservbyport_r():
           Since glibc 2.19:
               _DEFAULT_SOURCE
           glibc 2.19 and earlier:
               _BSD_SOURCE || _SVID_SOURCE

Will this change upset our glibc 2.18 users?

@fmayer
Copy link
Contributor Author

fmayer commented Mar 27, 2025

The man page says

       getservent_r(), getservbyname_r(), getservbyport_r():
           Since glibc 2.19:
               _DEFAULT_SOURCE
           glibc 2.19 and earlier:
               _BSD_SOURCE || _SVID_SOURCE

Will this change upset our glibc 2.18 users?

Doesn't that just mean the feature detection macro changed, not it was introduced in 2.19?

@fmayer fmayer merged commit aa3149d into main Mar 28, 2025
10 checks passed
@fmayer fmayer deleted the users/fmayer/spr/sanitizer-intercept-getservent_r-getservbyname_r-getservbyport_r branch March 28, 2025 00:05
@nico
Copy link
Contributor

nico commented Mar 28, 2025

Looks like this might've broken building on macOS: http://45.33.8.238/macm1/103291/step_3.txt

Can you take a look?

@Sharjeel-Khan
Copy link
Contributor

This also broke Android's ToT builds. It says error: invalid application of 'sizeof' to an incomplete type '__sanitizer_servent'

@fmayer
Copy link
Contributor Author

fmayer commented Mar 28, 2025

Sorry about that. Reverted #133358

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Mar 28, 2025
}

#if SANITIZER_INTERCEPT_GETSERVENT_R
INTERCEPTOR(int, getservent_r, struct __sanitizer_servent *result_buf,
Copy link
Collaborator

@vitalybuka vitalybuka Mar 28, 2025

Choose a reason for hiding this comment

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

changes in sanitizer_common should be tested in test/sanitizer_common.
msan tests can be added as complimentary to those

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.

7 participants