The branch, master has been updated via cb5d579 gitlab-ci: Add a 32bit build via df91870 Add fix for incorrect mapping of fcntl64() -> fcntl(), causing locking failures via ceb139d Add test for F_SETLK as this is needs to be 64-bit aware on 32-bit userspace from b15c02f tests: New test with poll
https://git.samba.org/?p=socket_wrapper.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit cb5d5790fff30e3be5a9465a85b1ac0aaaebfed2 Author: Andreas Schneider <a...@samba.org> Date: Fri May 5 07:14:26 2023 +0200 gitlab-ci: Add a 32bit build BUG: https://bugzilla.samba.org/show_bug.cgi?id=15367 Signed-off-by: Andreas Schneider <a...@samba.org> Reviewed-by: Andrew Bartlett <abart...@samba.org> commit df918708717c084ec9048be2864edcde81816108 Author: Andrew Bartlett <abart...@samba.org> Date: Fri May 5 13:34:00 2023 +1200 Add fix for incorrect mapping of fcntl64() -> fcntl(), causing locking failures We need to call fcntl64() if possible for 32-bit hosts This is a strange case of socket_wrapper breaking normal file operation. Newer glibc has introduced fcntl64 and symbol renaming but the end function call was not caught by the automatic rename. This means socket_wrapper would call fcntl(), not fcntl64 in libc and this would do a "struct flock" -> "struct flock64" translation on the supplied argument, despite this being already a flock64 from the caller. This in turn changed the lock offset values (eg to 0, 0). BUG: https://bugzilla.samba.org/show_bug.cgi?id=15367 Signed-off-by: Andrew Bartlett <abart...@samba.org> Reviewed-by: Andreas Schneider <a...@samba.org> commit ceb139dc42c50275a11ca974ef8800032cf24b6f Author: Andrew Bartlett <abart...@samba.org> Date: Fri May 5 13:15:51 2023 +1200 Add test for F_SETLK as this is needs to be 64-bit aware on 32-bit userspace If this is not correctly routed to fcntl64 (where that exists) then an incorrect thunking could be applied breaking the functionality. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15367 Signed-off-by: Andrew Bartlett <abart...@samba.org> Reviewed-by: Andreas Schneider <a...@samba.org> ----------------------------------------------------------------------- Summary of changes: .gitlab-ci.yml | 20 ++++++++++ ConfigureChecks.cmake | 1 + cmake/Toolchain-cross-m32.cmake | 23 +++++++++++ src/socket_wrapper.c | 25 ++++++++++++ tests/CMakeLists.txt | 1 + tests/test_fcntl_lock.c | 86 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 156 insertions(+) create mode 100644 cmake/Toolchain-cross-m32.cmake create mode 100644 tests/test_fcntl_lock.c Changeset truncated at 500 lines: diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d5dc461..ef98aeb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -224,6 +224,26 @@ tumbleweed/x86_64/clang: paths: - obj/ +tumbleweed/x86/gcc: + stage: test + image: $CI_REGISTRY/$BUILD_IMAGES_PROJECT:$TUMBLEWEED_BUILD + script: + - mkdir -p obj && cd obj && cmake + -DCMAKE_TOOLCHAIN_FILE=../cmake/Toolchain-cross-m32.cmake + -DCMAKE_BUILD_TYPE=RelWithDebInfo + -DPICKY_DEVELOPER=ON + -DUNIT_TESTING=ON .. && + make -j$(nproc) && ctest --output-on-failure + tags: + - shared + except: + - tags + artifacts: + expire_in: 1 week + when: on_failure + paths: + - obj/ + tumbleweed/static-analysis: stage: analysis image: $CI_REGISTRY/$BUILD_IMAGES_PROJECT:$TUMBLEWEED_BUILD diff --git a/ConfigureChecks.cmake b/ConfigureChecks.cmake index b820a65..c99e2ae 100644 --- a/ConfigureChecks.cmake +++ b/ConfigureChecks.cmake @@ -80,6 +80,7 @@ check_function_exists(__close_nocancel HAVE___CLOSE_NOCANCEL) check_function_exists(recvmmsg HAVE_RECVMMSG) check_function_exists(sendmmsg HAVE_SENDMMSG) check_function_exists(syscall HAVE_SYSCALL) +check_function_exists(fcntl64 HAVE_FCNTL64) if (UNIX) find_library(DLFCN_LIBRARY dl) diff --git a/cmake/Toolchain-cross-m32.cmake b/cmake/Toolchain-cross-m32.cmake new file mode 100644 index 0000000..7918c60 --- /dev/null +++ b/cmake/Toolchain-cross-m32.cmake @@ -0,0 +1,23 @@ +set(CMAKE_C_FLAGS "-m32" CACHE STRING "C compiler flags" FORCE) +set(CMAKE_CXX_FLAGS "-m32" CACHE STRING "C++ compiler flags" FORCE) + +set(LIB32 /usr/lib) # Fedora + +if(EXISTS /usr/lib32) + set(LIB32 /usr/lib32) # Arch, Solus +endif() + +set(CMAKE_SYSTEM_LIBRARY_PATH ${LIB32} CACHE STRING "system library search path" FORCE) +set(CMAKE_LIBRARY_PATH ${LIB32} CACHE STRING "library search path" FORCE) + +# this is probably unlikely to be needed, but just in case +set(CMAKE_EXE_LINKER_FLAGS "-m32 -L${LIB32}" CACHE STRING "executable linker flags" FORCE) +set(CMAKE_SHARED_LINKER_FLAGS "-m32 -L${LIB32}" CACHE STRING "shared library linker flags" FORCE) +set(CMAKE_MODULE_LINKER_FLAGS "-m32 -L${LIB32}" CACHE STRING "module linker flags" FORCE) + +# on Fedora and Arch and similar, point pkgconfig at 32 bit .pc files. We have +# to include the regular system .pc files as well (at the end), because some +# are not always present in the 32 bit directory +if(EXISTS ${LIB32}/pkgconfig) + set(ENV{PKG_CONFIG_LIBDIR} ${LIB32}/pkgconfig:/usr/share/pkgconfig:/usr/lib/pkgconfig:/usr/lib64/pkgconfig) +endiF() diff --git a/src/socket_wrapper.c b/src/socket_wrapper.c index bf4a976..de2f732 100644 --- a/src/socket_wrapper.c +++ b/src/socket_wrapper.c @@ -611,7 +611,11 @@ struct swrap_libc_symbols { SWRAP_SYMBOL_ENTRY(connect); SWRAP_SYMBOL_ENTRY(dup); SWRAP_SYMBOL_ENTRY(dup2); +#ifdef HAVE_FCNTL64 + SWRAP_SYMBOL_ENTRY(fcntl64); +#else SWRAP_SYMBOL_ENTRY(fcntl); +#endif SWRAP_SYMBOL_ENTRY(fopen); #ifdef HAVE_FOPEN64 SWRAP_SYMBOL_ENTRY(fopen64); @@ -978,7 +982,24 @@ static int libc_vfcntl(int fd, int cmd, va_list ap) arg = va_arg(ap, void *); + /* + * If fcntl64 exists then this is a system were fcntl is + * renamed (including when building this file), and so we must + * assume that the binary under test was built with + * -D_FILE_OFFSET_BITS=64 and pass on to fcntl64. + * + * If we are wrong, then fcntl is unwrapped, but while that is + * not ideal, is is also unlikely. + * + * In any case, it is always wrong to map fcntl64() to fcntl() + * as this will cause a thunk from struct flock -> flock64 + * that the caller had already prepared for. + */ +#ifdef HAVE_FCNTL64 + rc = swrap.libc.symbols._libc_fcntl64.f(fd, cmd, arg); +#else rc = swrap.libc.symbols._libc_fcntl.f(fd, cmd, arg); +#endif return rc; } @@ -1400,7 +1421,11 @@ static void __swrap_bind_symbol_all_once(void) swrap_bind_symbol_libsocket(connect); swrap_bind_symbol_libc(dup); swrap_bind_symbol_libc(dup2); +#ifdef HAVE_FCNTL64 + swrap_bind_symbol_libc(fcntl64); +#else swrap_bind_symbol_libc(fcntl); +#endif swrap_bind_symbol_libc(fopen); #ifdef HAVE_FOPEN64 swrap_bind_symbol_libc(fopen64); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b1a3c6c..e35c258 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -58,6 +58,7 @@ set(SWRAP_TESTS test_tcp_listen test_tcp_dup2 test_fcntl + test_fcntl_lock test_echo_tcp_connect test_echo_tcp_bind test_echo_tcp_socket_options diff --git a/tests/test_fcntl_lock.c b/tests/test_fcntl_lock.c new file mode 100644 index 0000000..98cf7c0 --- /dev/null +++ b/tests/test_fcntl_lock.c @@ -0,0 +1,86 @@ +#include "torture.h" + +#include <cmocka.h> +#include <stdlib.h> +#include <stdio.h> +#include <string.h> +#include <limits.h> +#include <unistd.h> +#include <fcntl.h> +#include <errno.h> + +static int setup(void **state) +{ + char test_tmpdir[256]; + const char *p; + + (void) state; /* unused */ + + snprintf(test_tmpdir, sizeof(test_tmpdir), "/tmp/test_socket_wrapper_XXXXXX"); + + p = mkdtemp(test_tmpdir); + assert_non_null(p); + + *state = strdup(p); + return 0; +} + +static int teardown(void **state) +{ + char remove_cmd[PATH_MAX] = {0}; + char *s = (char *)*state; + int rc; + + if (s == NULL) { + return -1; + } + + snprintf(remove_cmd, sizeof(remove_cmd), "rm -rf %s", s); + free(s); + + rc = system(remove_cmd); + if (rc < 0) { + fprintf(stderr, "%s failed: %s", remove_cmd, strerror(errno)); + } + + return rc; +} + +static void test_fcntl_lock(void **state) +{ + char file[PATH_MAX]; + int fd, rc; + struct flock lock; + char *s = (char *)*state; + + rc = snprintf(file, sizeof(file), "%s/file", s); + assert_in_range(rc, 0, PATH_MAX); + + fd = open(file, O_CREAT, 0600); + assert_return_code(fd, errno); + + lock.l_type = F_RDLCK; + lock.l_whence = SEEK_SET; + lock.l_start = 0; + lock.l_len = 4; + lock.l_pid = 0; + + rc = fcntl(fd, F_SETLK, &lock); + assert_return_code(rc, errno); + + rc = unlink(file); + assert_return_code(rc, errno); +} + + +int main(void) { + int rc; + + const struct CMUnitTest tcp_fcntl_lock_tests[] = { + cmocka_unit_test(test_fcntl_lock), + }; + + rc = cmocka_run_group_tests(tcp_fcntl_lock_tests, setup, teardown); + + return rc; +} -- Socket Wrapper Repository