bryancall commented on code in PR #13686:
URL: https://github.com/apache/trafficserver/pull/13686#discussion_r4017598827
##########
src/api/InkAPITest.cc:
##########
@@ -2084,8 +2084,14 @@ REGRESSION_TEST(SDK_API_TSfopen)(RegressionTest *test,
int /* atype ATS_UNUSED *
// Create unique tmp _file_name_, do not use any TS file_name
snprintf(write_file_name, PATH_NAME_MAX, "/tmp/%sXXXXXX",
ts::filename::PLUGIN);
- int write_file_fd; // this file will be reopened below
- if ((write_file_fd = mkstemp(write_file_name)) <= 0) {
+
+ // Narrow the umask across mkstemp so the temporary file cannot land in /tmp
+ // group or world accessible, whatever the process umask happens to be.
+ const mode_t old_umask = umask(S_IRWXG | S_IRWXO);
+ int write_file_fd = mkstemp(write_file_name); // this file will be
reopened below
+
+ umask(old_umask);
Review Comment:
Reverted in 61a005dcbb. The stale-restore hazard you point out is the part I
had not accounted for: umask() is not an atomic read-modify-write, so a
concurrent change in the window gets clobbered by the restore rather than
merely widened. Combined with mkstemp already guaranteeing 0600 on POSIX, the
narrowing bought nothing. CID 1022101 is left open for triage instead.
##########
src/api/InkAPITest.cc:
##########
@@ -2084,8 +2084,14 @@ REGRESSION_TEST(SDK_API_TSfopen)(RegressionTest *test,
int /* atype ATS_UNUSED *
// Create unique tmp _file_name_, do not use any TS file_name
snprintf(write_file_name, PATH_NAME_MAX, "/tmp/%sXXXXXX",
ts::filename::PLUGIN);
- int write_file_fd; // this file will be reopened below
- if ((write_file_fd = mkstemp(write_file_name)) <= 0) {
+
+ // Narrow the umask across mkstemp so the temporary file cannot land in /tmp
+ // group or world accessible, whatever the process umask happens to be.
+ const mode_t old_umask = umask(S_IRWXG | S_IRWXO);
+ int write_file_fd = mkstemp(write_file_name); // this file will be
reopened below
+
+ umask(old_umask);
Review Comment:
Reverted in 61a005dcbb, along with the reasoning in the reply to Copilot
above. CID 1022101 will be triaged in Coverity rather than patched.
##########
src/api/InkAPITest.cc:
##########
@@ -7996,9 +8002,7 @@ load(const char *append_string)
p = TSIOBufferBlockWriteStart(blk, &avail);
ink_strlcpy(p, append_string, avail);
- if (append_string != nullptr) {
- TSIOBufferProduce(append_buffer, strlen(append_string));
- }
+ TSIOBufferProduce(append_buffer, strlen(append_string));
Review Comment:
Added in 400491043d as `TSAssert(nullptr != append_string)` at the top of
`load()`. That states the precondition the single caller already satisfies,
which is what the removed check was gesturing at without enforcing.
##########
src/proxy/http/remap/unit-tests/test_PluginFactory.cc:
##########
@@ -395,7 +395,7 @@ SCENARIO("loading plugins", "[plugin][core]")
WHEN("config using nonexisting absolute plugin file name")
{
- fs::path relativeExistingPath = pluginName;
+ fs::path relativeExistingPath = std::move(pluginName);
Review Comment:
Added in e52ef03210. It was compiling through a transitive include.
--
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]
