Repository navigation
Conversation
They were brought by 20f9dd6 but were unfortunately degraded over time. Only clone the original properties if the new file has the same uid and gid. Reported-by: Stanislav Fort
|
Analysis of PR #22843 at 4115a2c1: Test ../../tests/http/test_19_shutdown.py::TestShutdown::test_19_03_shutdown_by_server[http/1.1] failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Generated by Testclutch |
There was a problem hiding this comment.
🟢 Approval recommended
The ownership checks and build-system feature detection consistently restore the intended secure behavior.
Pull request overview
Restores secure permission inheritance for temporary files used by cookie, HSTS, and Alt-Svc persistence.
Changes:
- Creates temporary files with restrictive permissions.
- Copies the original mode only when UID and GID match.
- Detects
fchmodin CMake and Autoconf builds.
File summaries
| File | Description |
|---|---|
lib/curl_fopen.c |
Restores ownership-gated mode cloning. |
lib/curl_config-cmake.h.in |
Adds the CMake feature macro. |
configure.ac |
Adds Autoconf detection. |
CMakeLists.txt |
Checks for fchmod. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Detection can be omitted for Windows, since there is no uid/gid there, nor this function. Also prefill on Unix in CI, since it's supported everywhere: diff --git a/CMake/unix-cache.cmake b/CMake/unix-cache.cmake
index 8ec2cd48c1..36c75e7daa 100644
--- a/CMake/unix-cache.cmake
+++ b/CMake/unix-cache.cmake
@@ -80,6 +80,7 @@ if((APPLE AND CMAKE_OSX_DEPLOYMENT_TARGET VERSION_GREATER_EQUAL 10.9) OR
elseif(NOT APPLE)
set(HAVE_MEMSET_S 0)
endif()
+set(HAVE_FCHMOD 1)
set(HAVE_FCNTL 1)
set(HAVE_FCNTL_H 1)
set(HAVE_FCNTL_O_NONBLOCK 1)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e2512b1491..5fb2e22adb 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1615,7 +1615,6 @@ endif()
check_function_exists("accept4" HAVE_ACCEPT4)
check_function_exists("fnmatch" HAVE_FNMATCH)
-check_function_exists("fchmod" HAVE_FCHMOD)
check_symbol_exists("basename" "${CURL_INCLUDES};string.h" HAVE_BASENAME) # libgen.h unistd.h
check_function_exists("poll" HAVE_POLL) # poll.h
check_symbol_exists("socket" "${CURL_INCLUDES}" HAVE_SOCKET) # winsock2.h sys/socket.h
@@ -1670,6 +1669,7 @@ endif()
if(NOT WIN32)
check_include_file("dirent.h" HAVE_DIRENT_H)
+ check_function_exists("fchmod" HAVE_FCHMOD)
check_function_exists("gettimeofday" HAVE_GETTIMEOFDAY) # sys/time.h
check_function_exists("if_nametoindex" HAVE_IF_NAMETOINDEX) # net/if.h
check_function_exists("realpath" HAVE_REALPATH)
diff --git a/configure.ac b/configure.ac
index 042d2defb2..9a382c321e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4220,7 +4220,6 @@ esac
AC_CHECK_FUNCS([\
accept4 \
eventfd \
- fchmod \
fnmatch \
geteuid \
getpass_r \
@@ -4250,6 +4249,7 @@ fi
if test "$curl_cv_native_windows" != "yes"; then
AC_CHECK_FUNCS([\
+ fchmod \
gettimeofday \
if_nametoindex \
realpath \ |
|
@vszakats feel free to push that fix to the branch! |
|
This brings back a warning-as-error for 32-bit Android for the third time 😆 Originally fixed here: Maybe we need to fix it more sustainably somehow? Sorry for being late, I've just returned from vacation and am now testing the v8.23 RCs. |
|
Created an issue out of my comment for visibility: |
Add `CURLX_FILE_MODE(mode)` macro which casts the value to `mode_t` on
32-bit Android and passes it through as-is on other platforms.
Replace existing workaround with it, and apply to the recently
reintroduced `fchmod()` call that missed it.
Fixing:
```
lib/curl_fopen.c:146:24: error: implicit conversion loses integer precision: 'unsigned int' to 'mode_t' (aka 'unsigned short') [-Wimplicit-int-conversion]
146 | if(fchmod(fd, sb.st_mode) == -1)
| ~~~~~~ ~~~^~~~~~~
```
Ref: https://github.com/curl/curl/actions/runs/36549680315/job/109344511716?pr=23159#step:8:30
Ref: https://android.googlesource.com/platform/bionic/+/refs/tags/ndk-r25c/libc/include/sys/stat.h#86
Reported-by: Marcel Raad
Fixes #23157
Follow-up to 83b7ad0 #22843
Follow-up to f0eacd9 #12998
Follow-up to 03cb1ff #12395 #12299
Follow-up to 06dc599 #11313
Closes #23158
They were brought by 20f9dd6 but were unfortunately degraded over time. Only clone the original properties if the new file has the same uid and gid.
Reported-by: Stanislav Fort