jnioche commented on PR #9187: URL: https://github.com/apache/storm/pull/9187#issuecomment-6336283524
AI assisted review as my C skills are non-existent Verdict: approve, with two small suggestions. What I checked (gcc 13 / glibc 2.39, -O2 -D_FORTIFY_SOURCE=2, both gnu17 and gnu2x): Base: fails with 10 -Werror errors: 3 for strncpy truncation; 6 for ignored return values of setuid, setgid, system and seteuid; 1 for desc possibly used uninitialized. PR: builds clean, and the native test suite passes when run as an unprivileged user. glibc 2.43 strrchr case: I couldn’t reproduce it, since there’s no gcc 15 here. The oci.c call is the only one in the launcher that takes a const argument; the two strchr calls work on non-const buffers. Correctness The memcpy replacements are safe: the lengths come from strlen and the destinations are already zeroed with room for the terminator. Initializing desc fixes a real bug. When the strdup of the run root failed, the code jumped to mem_fail, which later ran free(desc) on an uninitialized pointer. Checking setuid/setgid now fails closed. That matters most before run_oci_container, which previously could carry on without root. Suggestions Worse error for a non-setuid binary. If the launcher isn’t setuid root, it now exits with “unable to set user to root - Operation not permitted”. Previously check_executor_permissions ran first and printed “should be user-owned by root”. That’s a common misconfiguration, so either run the permissions check first or add a hint to the new message. Misleading test cleanup message. chmod ...; rm -fr ... returns rm’s exit status, and rm -fr returns 0 when the directory is missing. So “No previous test directory to clean up” only prints when rm fails, for example on a leftover root-owned directory, which I hit. It should say something like “WARNING: could not clean up previous test directory”. Nit: use %u for gid_t. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
