Can we prefix compat symbols to avoid symbol clashes in static links?

[botovq]

While we no longer export the compat symbols from the dynamic libraries, there is still an issue with static linking, see e.g., curl/curl#12257

Can we leverage a trick like the one used in the hidden directories to prefix these symbols transparently?

[botovq]

I wonder if we can do something like this in the various compat headers:

#ifndef HAVE_ARC4RANDOM
#define arc4random _libressl_arc4random
uint32_t arc4random(void);
#endif

This way we don't need to litter the main source code with prefixes and the symbols should no longer leak out of the libraries, even with static linking.

[bob-beck]

Is it really only the arc4random that becomes a problem here now that some systems have it?

[botovq]

On Fri, Nov 10, 2023 at 06:11:19AM -0800, Bob Beck wrote: Is it really only the arc4random that becomes a problem here now that some systems have it?

All compat symbols are a problem. And there may be internal symbols that could also potentially clash.

[vszakats]

Another non-prefixed symbol, strtonum is causing a number of issues on macOS:

1. autotools detects it even if the OS target version doesn't offer it. When referenced
   from the code, it results in compiler warnings with LibreSSL 3.7.3 (gcc 9, macOS, mingw-w64) #910 (comment).

note: 'strtonum' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.9.0`

The workaround for that is to suppress detection with export ac_cv_func_strtonum='no'.
That shifts the problem into the next ones.

2. cmake doesn't make an attempt to auto-detect strtonum, by default assuming
   that the function isn't provided. But since it is in fact provided via stdlib.h (with
   the caveat above), these warnings appear when using it:

libressl/crypto/x509/x509_addr.c:1645:17: warning: 'strtonum' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
                        prefix_len = strtonum(s + i2, 0, 8 * length, &errstr);
                                     ^~~~~~~~
libressl/crypto/../include/compat/stdlib.h:43:11: note: 'strtonum' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.9.0
long long strtonum(const char *nptr, long long minval,
          ^
libressl/crypto/x509/x509_addr.c:1645:17: note: enclose 'strtonum' in a __builtin_available check to silence this warning
                        prefix_len = strtonum(s + i2, 0, 8 * length, &errstr);
                                     ^~~~~~~~

https://github.com/curl/curl-for-win/actions/runs/7159857921/job/19493576239#step:3:3226
Setting HAVE_STRTONUM would take use back to point 1.

3. Possibly another fallout of point 2 is that the strtonum function defined by LibreSSL
   leaks from the shared library, despite building with -fvisibility=hidden:

00000000001ee734 T _strtonum

https://github.com/curl/curl-for-win/actions/runs/7159857921/job/19493576239#step:3:5300
I haven't found a workaround for this yet.

[botovq]

The point of this issue is that all compat symbols should be prefixed. The problem is that the Windows API needs to be dealt with by someone familiar with it. I sent @busterb what I have. It will happen at some point.

Regarding 2, for strtonum, a stanza like for strsep should be added to CMakeLists.txt . That seems independent of this issue.

I'm not sure what to make of the first and the third problem. Both seem unrelated to this issue?

[vszakats]

I believe all these are related to re-using existing symbol names. It isn't Windows-specific either.

In case of strtonum, there are issues with HAVE_STRTONUM either being set and unset. In fact adding the missing feature check to CMakeLists.txt would introduce the same problem as already exists with autotools (in point 1). That is, it's mis-detecting strtonum as always present because the SDK headers declare it. But the SDK offers it only for macOS 11 and newer. This is ignored by both CMake [1] and autotools when doing the detection. Thus, when targeting an older macOS version, warning in point 1 appears. The resulting binary then fails to run on the targeted (older) macOS versions because strtonum is not provided by those OS versions.

This can only be fixed by building without HAVE_STRTONUM, but in that case LibreSSL provides its own non-namespaced implementation, which causes the warnings in point 2, and (possibly) the symbol leak in point 3.

In summary, in case of strtonum the only issue-free scenario on macOS is when building for macOS 11 or newer (which also needs a manual setting of -DHAVE_STRTONUM now with CMake).

[1]:
with this added to CMakeLists.txt:

check_function_exists(strtonum HAVE_STRTONUM)
if(HAVE_STRTONUM)
  add_definitions(-DHAVE_STRTONUM)
endif()

$ cmake -B bld -Wno-dev -DCMAKE_BUILD_TYPE=Release -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9 \
  -DCMAKE_OSX_ARCHITECTURES=x86_64 -DCMAKE_INSTALL_MESSAGE=NEVER -DCMAKE_INSTALL_PREFIX=/usr \
  -DCMAKE_C_COMPILER=clang -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk \
  -DCMAKE_C_COMPILER_TARGET=x86_64-apple-darwin -DCMAKE_SYSTEM_PROCESSOR=x86_64 -DBUILD_SHARED_LIBS=OFF \
  -DLIBRESSL_APPS=OFF -DLIBRESSL_TESTS=OFF '-DCMAKE_C_FLAGS= -Wno-unused-command-line-argument \
  -mmacosx-version-min=10.9 -fvisibility=hidden -fno-unwind-tables -fno-asynchronous-unwind-tables \
  -ffile-prefix-map=[...]/libressl= -Wa,--noexecstack -Dglobl=private_extern
-- The C compiler identification is AppleClang 14.0.0.14000029
[...]
-- Looking for strtonum
-- Looking for strtonum - found
[...]
[ 89%] Building C object crypto/CMakeFiles/crypto_obj.dir/x509/x509_alt.c.o
[...]libressl/crypto/x509/x509_addr.c:1645:17: warning: 'strtonum' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
                        prefix_len = strtonum(s + i2, 0, 8 * length, &errstr);
                                     ^~~~~~~~
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/stdlib.h:356:2: note: 'strtonum' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.9.0
        strtonum(const char *__numstr, long long __minval, long long __maxval, const char **__errstrp)
        ^
[...]libressl/crypto/x509/x509_addr.c:1645:17: note: enclose 'strtonum' in a __builtin_available check to silence this warning
                        prefix_len = strtonum(s + i2, 0, 8 * length, &errstr);
                                     ^~~~~~~~

[botovq]

I understand that your problem isn't Windows-specific. What I'm saying is that the reason this issue isn't moving forward is Windows-specific.

Otherwise I think we conflate two independent issues:

1. If I understand you right, autotools and CMake misdetect presence of strtonum and no matter what we call the compat symbol, we'll have a problem on macOS 10.9, no? So the feature detection should not only check for a declaration but actually try to link a program with strtonum, I suppose.
2. The symbol clash that is issue wants to address, but I don't see anything specific to strtonum in here.

[botovq]

If it helps, here's what I currently have for the prefixing:

diff --git a/include/compat/stdio.h b/include/compat/stdio.h
index d5725c9..4ddd63a 100644
--- a/include/compat/stdio.h
+++ b/include/compat/stdio.h
@@ -20,7 +20,9 @@
 
 #ifndef HAVE_ASPRINTF
 #include <stdarg.h>
+#define vasprintf libressl_vasprintf
 int vasprintf(char **str, const char *fmt, va_list ap);
+#define asprintf libressl_asprintf
 int asprintf(char **str, const char *fmt, ...);
 #endif
 
diff --git a/include/compat/stdlib.h b/include/compat/stdlib.h
index 2eaea24..76dc07c 100644
--- a/include/compat/stdlib.h
+++ b/include/compat/stdlib.h
@@ -20,26 +20,36 @@
 #include <stdint.h>
 
 #ifndef HAVE_ARC4RANDOM_BUF
+#define arc4random libressl_arc4random
 uint32_t arc4random(void);
+#define arc4random_buf libressl_arc4random_buf
 void arc4random_buf(void *_buf, size_t n);
+#define arc4random_uniform libressl_arc4random_uniform
 uint32_t arc4random_uniform(uint32_t upper_bound);
 #endif
 
 #ifndef HAVE_FREEZERO
+#define freezero libressl_freezero
 void freezero(void *ptr, size_t sz);
 #endif
 
 #ifndef HAVE_GETPROGNAME
+#define getprogname libressl_getprogname
 const char * getprogname(void);
 #endif
 
+#ifndef HAVE_REALLOCARRAY
+#define reallocarray libressl_reallocarray
 void *reallocarray(void *, size_t, size_t);
+#endif
 
 #ifndef HAVE_RECALLOCARRAY
+#define recallocarray libressl_recallocarray
 void *recallocarray(void *, size_t, size_t, size_t);
 #endif
 
 #ifndef HAVE_STRTONUM
+#define strtonum libressl_strtonum
 long long strtonum(const char *nptr, long long minval,
 		long long maxval, const char **errstr);
 #endif
diff --git a/include/compat/string.h b/include/compat/string.h
index 4bf7519..7e0245a 100644
--- a/include/compat/string.h
+++ b/include/compat/string.h
@@ -27,43 +27,54 @@
 #endif
 
 #ifndef HAVE_STRCASECMP
+#define strcasecmp libressl_strcasecmp
 int strcasecmp(const char *s1, const char *s2);
+#define strncasecmp libressl_strncasecmp
 int strncasecmp(const char *s1, const char *s2, size_t len);
 #endif
 
 #ifndef HAVE_STRLCPY
+#define strlcpy libressl_strlcpy
 size_t strlcpy(char *dst, const char *src, size_t siz);
 #endif
 
 #ifndef HAVE_STRLCAT
+#define strlcat libressl_strlcat
 size_t strlcat(char *dst, const char *src, size_t siz);
 #endif
 
 #ifndef HAVE_STRNDUP
+#define strndup libressl_strndup
 char * strndup(const char *str, size_t maxlen);
 /* the only user of strnlen is strndup, so only build it if needed */
 #ifndef HAVE_STRNLEN
+#define strnlen libressl_strnlen
 size_t strnlen(const char *str, size_t maxlen);
 #endif
 #endif
 
 #ifndef HAVE_STRSEP
+#define strsep libressl_strsep
 char *strsep(char **stringp, const char *delim);
 #endif
 
 #ifndef HAVE_EXPLICIT_BZERO
+#define explicit_bzero libressl_explicit_bzero
 void explicit_bzero(void *, size_t);
 #endif
 
 #ifndef HAVE_TIMINGSAFE_BCMP
+#define timingsafe_bcmp libressl_timingsafe_bcmp
 int timingsafe_bcmp(const void *b1, const void *b2, size_t n);
 #endif
 
 #ifndef HAVE_TIMINGSAFE_MEMCMP
+#define timingsafe_memcmp libressl_timingsafe_memcmp
 int timingsafe_memcmp(const void *b1, const void *b2, size_t len);
 #endif
 
 #ifndef HAVE_MEMMEM
+#define memmem libressl_memmem
 void * memmem(const void *big, size_t big_len, const void *little,
 	size_t little_len);
 #endif

[vszakats]

Got it, though I'm missing which Windows API-issue is holding this back?

1. Yes, the strtonum mis-detection is not strictly related. Though with a namespaced local implementation, the issue would reduce to a minor one.
2. Indeed, the symbol clash issue isn't strtonum-specific. Sometimes the consequences fall far the root cause (like the leaking hidden symbols, still with an if), so I though it might be interesing to document some cases coming up in practice.

[vszakats]

@botovq Thanks for that! It should fix all the problems around this.

This macOS test run confirms it fixing the warnings and symbol leak:
https://github.com/curl/curl-for-win/actions/runs/7170054527/job/19521898523

Compared to this:
https://github.com/curl/curl-for-win/actions/runs/7160876447/job/19495739576
https://github.com/curl/curl-for-win/actions/runs/7160876447/job/19495739576#step:3:3004
https://github.com/curl/curl-for-win/actions/runs/7160876447/job/19495739576#step:3:5078

If this can make it to the next release, we might also address the CMake detection
issue without worrying about it being unprecise on macOS:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -234,6 +234,11 @@ if(HAVE_STRSEP)
 	add_definitions(-DHAVE_STRSEP)
 endif()
 
+check_function_exists(strtonum HAVE_STRTONUM)
+if(HAVE_STRTONUM)
+	add_definitions(-DHAVE_STRTONUM)
+endif()
+
 check_function_exists(timegm HAVE_TIMEGM)
 if(HAVE_TIMEGM)
 	add_definitions(-DHAVE_TIMEGM)

[botovq]

@vszakats many thanks for your help. #961 can be merged once it's approved by a member of the project, and then we can merge #963, too. The remaining symbols can be taken care of later.

Got it, though I'm missing which Windows API-issue is holding this back?

It's not a specific API, but it will have to be done by someone familiar enough with Windows to understand the complications in the compat layer. It is very foreign to me.

[vszakats]

Thanks @botovq! Looking forward for these.

In the meantime if you have any specific pointer about Windows-specific compat layer complications, I'd be glad reading into it.

[botovq]

The first thing to untangle probably is NO_REDEF_POSIX_FUNCTIONS. Is this even needed if we prefix all compat functions with libressl_? Once this is figured out, it probably only is a matter of going through all the compat headers and continue what I've done in #961.

[vszakats]

NO_REDEF_POSIX_FUNCTIONS seems to be fixing a subset of this exact issue. I don't see a reason why it's needed if everything is prefixed. Generic names like read / send / close may need function-style redefinitions if they happen to pick up a non-function keyword or variable name.