This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new bc4c70c78 more strict check for [un]setenv() return code
bc4c70c78 is described below
commit bc4c70c78435bd65a385eeb22f3894457d0b2e3a
Author: Alexey Serbin <[email protected]>
AuthorDate: Fri Dec 1 18:23:12 2023 -0800
more strict check for [un]setenv() return code
Even if setenv() and unsetenv() function expected to almost never fail
with the current code as per documentation [1], it still makes sense
to check for their return code. This patch updates the call sites
of these functions accordingly.
I also addressed a few warnings that ClangTidy produced
on the updated code.
[1] https://man7.org/linux/man-pages/man3/setenv.3.html
Change-Id: I8257bc4367b3fdf0cda5171de9b39c8d8b87fba9
Reviewed-on: http://gerrit.cloudera.org:8080/20744
Reviewed-by: Abhishek Chennaka <[email protected]>
Reviewed-by: Mahesh Reddy <[email protected]>
Tested-by: Kudu Jenkins
Reviewed-by: Yifan Zhang <[email protected]>
---
src/kudu/benchmarks/tpch/tpch_real_world.cc | 2 +-
src/kudu/client/client-test.cc | 6 +++---
src/kudu/rpc/negotiation-test.cc | 10 +++++-----
src/kudu/security/init.cc | 16 ++++++++++------
src/kudu/security/test/mini_kdc.cc | 2 +-
src/kudu/server/webserver-test.cc | 2 +-
src/kudu/tools/kudu-tool-test.cc | 23 ++++++++++++++---------
src/kudu/util/subprocess.cc | 3 +--
src/kudu/util/test_util.cc | 6 +++---
src/kudu/util/test_util.h | 6 +++---
10 files changed, 42 insertions(+), 34 deletions(-)
diff --git a/src/kudu/benchmarks/tpch/tpch_real_world.cc
b/src/kudu/benchmarks/tpch/tpch_real_world.cc
index 76d8326a0..905e11d01 100644
--- a/src/kudu/benchmarks/tpch/tpch_real_world.cc
+++ b/src/kudu/benchmarks/tpch/tpch_real_world.cc
@@ -243,7 +243,7 @@ Status TpchRealWorld::CreateFifos() {
Status TpchRealWorld::StartDbgens() {
for (int i = 1; i <= FLAGS_tpch_num_inserters; i++) {
// This environment variable is necessary if dbgen isn't in the current
dir.
- setenv("DSS_CONFIG", FLAGS_tpch_path_to_dbgen_dir.c_str(), 1);
+ PCHECK(setenv("DSS_CONFIG", FLAGS_tpch_path_to_dbgen_dir.c_str(), 1) == 0);
vector<string> argv;
argv.push_back(Substitute("$0/dbgen", FLAGS_tpch_path_to_dbgen_dir));
argv.emplace_back("-q");
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 0168ddc95..1078c6884 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -7670,19 +7670,19 @@ TEST_F(ClientTest, TestInvalidPartitionerBuilder) {
// and it reflects to the FLAGS_v.
TEST_F(ClientTest, TestVerboseLevelByEnvVar) {
FLAGS_v = 0;
- setenv(kVerboseEnvVar, "5", 1); // 1 = overwrite if variable already exists.
+ PCHECK(setenv(kVerboseEnvVar, "5", 1) == 0); // 1 = overwrite if variable
already exists.
SetVerboseLevelFromEnvVar();
ASSERT_EQ(5, FLAGS_v);
// negative values are to be ignored.
FLAGS_v = 0;
- setenv(kVerboseEnvVar, "-1", 1);
+ PCHECK(setenv(kVerboseEnvVar, "-1", 1) == 0);
SetVerboseLevelFromEnvVar();
ASSERT_EQ(0, FLAGS_v);
// non-parsable values are to be ignored.
FLAGS_v = 0;
- setenv(kVerboseEnvVar, "abc", 1);
+ PCHECK(setenv(kVerboseEnvVar, "abc", 1) == 0);
SetVerboseLevelFromEnvVar();
ASSERT_EQ(0, FLAGS_v);
}
diff --git a/src/kudu/rpc/negotiation-test.cc b/src/kudu/rpc/negotiation-test.cc
index eee35ebc5..2ef5929a7 100644
--- a/src/kudu/rpc/negotiation-test.cc
+++ b/src/kudu/rpc/negotiation-test.cc
@@ -332,7 +332,7 @@ TEST_P(TestNegotiation, TestNegotiation) {
// Create the server principal and keytab.
string kt_path;
ASSERT_OK(kdc.CreateServiceKeytab("kudu/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/) == 0);
server_negotiation.set_server_fqdn("127.0.0.1");
ASSERT_OK(server_negotiation.EnableGSSAPI());
break;
@@ -1434,7 +1434,7 @@ TEST_F(TestNegotiation, TestGSSAPIInvalidNegotiation) {
// Create the server principal and keytab.
string kt_path;
ASSERT_OK(kdc.CreateServiceKeytab("kudu/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/) == 0);
// Try to negotiate with no krb5 credentials on the client. It should fail
on both
// sides.
@@ -1469,7 +1469,7 @@ TEST_F(TestNegotiation, TestGSSAPIInvalidNegotiation) {
// credentials.
// Authentication should now fail.
ASSERT_OK(kdc.CreateServiceKeytab("otherservice/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/) == 0);
RunNegotiationTest(
[](unique_ptr<Socket> socket) {
@@ -1517,7 +1517,7 @@ TEST_F(TestNegotiation, TestPreflight) {
ASSERT_OK(kdc.SetKrb5Environment());
string kt_path;
ASSERT_OK(kdc.CreateServiceKeytab("kudu/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/) == 0);
ASSERT_OK(ServerNegotiation::PreflightCheckGSSAPI("kudu"));
@@ -1537,7 +1537,7 @@ TEST_F(TestNegotiation, TestPreflight) {
// Try with a keytab that has the wrong credentials.
ASSERT_OK(kdc.CreateServiceKeytab("wrong-service/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1 /*replace*/) == 0);
s = ServerNegotiation::PreflightCheckGSSAPI("kudu");
ASSERT_FALSE(s.ok());
#ifndef KRB5_VERSION_LE_1_10
diff --git a/src/kudu/security/init.cc b/src/kudu/security/init.cc
index b4bbad6e1..33b0b8e95 100644
--- a/src/kudu/security/init.cc
+++ b/src/kudu/security/init.cc
@@ -497,19 +497,23 @@ Status Krb5ParseName(const string& principal, string*
service_name,
return Status::OK();
}
-Status InitKerberosForServer(const std::string& raw_principal, const
std::string& keytab_file,
- const std::string& krb5ccname, bool disable_krb5_replay_cache) {
- if (keytab_file.empty()) return Status::OK();
+Status InitKerberosForServer(const std::string& raw_principal,
+ const std::string& keytab_file,
+ const std::string& krb5ccname,
+ bool disable_krb5_replay_cache) {
+ if (keytab_file.empty()) {
+ return Status::OK();
+ }
- setenv("KRB5CCNAME", krb5ccname.c_str(), 1);
- setenv("KRB5_KTNAME", keytab_file.c_str(), 1);
+ PCHECK(setenv("KRB5CCNAME", krb5ccname.c_str(), 1) == 0);
+ PCHECK(setenv("KRB5_KTNAME", keytab_file.c_str(), 1) == 0);
if (disable_krb5_replay_cache) {
// KUDU-1897: disable the Kerberos replay cache. The KRPC protocol
includes a
// per-connection server-generated nonce to protect against replay attacks
// when authenticating via Kerberos. The replay cache has many performance
and
// implementation issues.
- setenv("KRB5RCACHETYPE", "none", 1);
+ PCHECK(setenv("KRB5RCACHETYPE", "none", 1) == 0);
}
g_kinit_ctx = new KinitContext();
diff --git a/src/kudu/security/test/mini_kdc.cc
b/src/kudu/security/test/mini_kdc.cc
index 12abe5c7c..c7bcee333 100644
--- a/src/kudu/security/test/mini_kdc.cc
+++ b/src/kudu/security/test/mini_kdc.cc
@@ -357,7 +357,7 @@ Status MiniKdc::SetKrb5Environment() const {
return Status::IllegalState("KDC not started");
}
for (const auto& p : GetEnvVars()) {
- CHECK_ERR(setenv(p.first.c_str(), p.second.c_str(), 1 /*overwrite*/));
+ PCHECK(setenv(p.first.c_str(), p.second.c_str(), 1 /*overwrite*/) == 0);
}
return Status::OK();
diff --git a/src/kudu/server/webserver-test.cc
b/src/kudu/server/webserver-test.cc
index a86016258..472a64e62 100644
--- a/src/kudu/server/webserver-test.cc
+++ b/src/kudu/server/webserver-test.cc
@@ -205,7 +205,7 @@ class SpnegoWebserverTest : public WebserverTest {
kdc_->SetKrb5Environment();
string kt_path;
ASSERT_OK(kdc_->CreateServiceKeytab("HTTP/127.0.0.1", &kt_path));
- CHECK_ERR(setenv("KRB5_KTNAME", kt_path.c_str(), 1));
+ PCHECK(setenv("KRB5_KTNAME", kt_path.c_str(), 1) == 0);
ASSERT_OK(kdc_->CreateUserPrincipal("alice"));
opts->require_spnego = true;
diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc
index db21dec69..803c8bf17 100644
--- a/src/kudu/tools/kudu-tool-test.cc
+++ b/src/kudu/tools/kudu-tool-test.cc
@@ -18,6 +18,7 @@
#include <sys/stat.h>
#include <algorithm>
+#include <cerrno>
#include <climits>
#include <cstdint>
#include <cstdio>
@@ -2014,7 +2015,7 @@ TEST_F(ToolTest, TestPbcTools) {
StrCat("#!/usr/bin/env bash\n", editor_shell),
editor_path));
chmod(editor_path.c_str(), 0755);
- setenv("EDITOR", editor_path.c_str(), /* overwrite */1);
+ PCHECK(setenv("EDITOR", editor_path.c_str(), /* overwrite */1) == 0);
return RunTool(Substitute("pbc edit $0 $1", extra_flags, instance_path),
stdout, stderr, nullptr, nullptr);
};
@@ -2251,7 +2252,7 @@ TEST_F(ToolTest, TestPbcToolsOnMultipleBlocks) {
StrCat("#!/usr/bin/env bash\n", editor_shell),
editor_path));
chmod(editor_path.c_str(), 0755);
- setenv("EDITOR", editor_path.c_str(), /* overwrite */1);
+ PCHECK(setenv("EDITOR", editor_path.c_str(), /* overwrite */1) == 0);
return RunTool(Substitute("pbc edit $0 $1 $2", extra_flags, metadata_path,
encryption_args),
stdout, stderr, nullptr, nullptr);
};
@@ -8845,7 +8846,11 @@ TEST_F(ToolTest, TestParseMetrics) {
TEST_F(ToolTest, ClusterNameResolverEnvNotSet) {
const string kClusterName = "external_mini_cluster";
- CHECK_ERR(unsetenv("KUDU_CONFIG"));
+ if (unsetenv("KUDU_CONFIG") != 0) {
+ // unsetenv() on macOS might return EINVAL if the specified variable
+ // was not present in the environment of the process.
+ PCHECK(errno == EINVAL);
+ }
string stderr;
Status s = RunActionStderrString(
Substitute("master list @$0", kClusterName),
@@ -8857,9 +8862,9 @@ TEST_F(ToolTest, ClusterNameResolverEnvNotSet) {
TEST_F(ToolTest, ClusterNameResolverFileNotExist) {
const string kClusterName = "external_mini_cluster";
- CHECK_ERR(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1));
+ PCHECK(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1) == 0);
SCOPED_CLEANUP({
- CHECK_ERR(unsetenv("KUDU_CONFIG"));
+ PCHECK(unsetenv("KUDU_CONFIG") == 0);
});
string stderr;
@@ -8880,9 +8885,9 @@ TEST_F(ToolTest, ClusterNameResolverFileCorrupt) {
// Prepare ${KUDU_CONFIG}.
const string kClusterName = "external_mini_cluster";
- CHECK_ERR(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1));
+ PCHECK(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1) == 0);
SCOPED_CLEANUP({
- CHECK_ERR(unsetenv("KUDU_CONFIG"));
+ PCHECK(unsetenv("KUDU_CONFIG") == 0);
});
// Missing 'clusters_info' section.
@@ -8923,9 +8928,9 @@ TEST_F(ToolTest, ClusterNameResolverNormal) {
// Prepare ${KUDU_CONFIG}.
const string kClusterName = "external_mini_cluster";
- CHECK_ERR(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1));
+ PCHECK(setenv("KUDU_CONFIG", GetTestDataDirectory().c_str(), 1) == 0);
SCOPED_CLEANUP({
- CHECK_ERR(unsetenv("KUDU_CONFIG"));
+ PCHECK(unsetenv("KUDU_CONFIG") == 0);
});
string content = Substitute(
diff --git a/src/kudu/util/subprocess.cc b/src/kudu/util/subprocess.cc
index 2228b98f6..8130c07b9 100644
--- a/src/kudu/util/subprocess.cc
+++ b/src/kudu/util/subprocess.cc
@@ -41,7 +41,6 @@
#include <glog/raw_logging.h>
#include <glog/stl_logging.h>
-#include "kudu/gutil/basictypes.h"
#include "kudu/gutil/port.h"
#include "kudu/gutil/strings/join.h"
#include "kudu/gutil/strings/numbers.h"
@@ -481,7 +480,7 @@ Status Subprocess::Start() {
// variant of exec to do $PATH searching if the executable specified
// by the caller isn't an absolute path.
for (const auto& env : env_) {
- ignore_result(setenv(env.first.c_str(), env.second.c_str(), 1 /*
overwrite */));
+ PCHECK(setenv(env.first.c_str(), env.second.c_str(), 1 /* overwrite */)
== 0);
}
execvp(program_.c_str(), &argv_ptrs[0]);
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index 1be36160b..b491018fc 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -200,9 +200,9 @@ void KuduTest::OverrideKrb5Environment() {
//
// NOTE: we don't simply *unset* the variables, because then we'd still pick
up
// the user's /etc/krb5.conf and other default locations.
- setenv("KRB5_CONFIG", kInvalidPath, 1);
- setenv("KRB5_KTNAME", kInvalidPath, 1);
- setenv("KRB5CCNAME", kInvalidPath, 1);
+ PCHECK(setenv("KRB5_CONFIG", kInvalidPath, 1) == 0);
+ PCHECK(setenv("KRB5_KTNAME", kInvalidPath, 1) == 0);
+ PCHECK(setenv("KRB5CCNAME", kInvalidPath, 1) == 0);
}
void KuduTest::SetEncryptionFlags(bool enable_encryption) {
diff --git a/src/kudu/util/test_util.h b/src/kudu/util/test_util.h
index 959e0c216..15289b492 100644
--- a/src/kudu/util/test_util.h
+++ b/src/kudu/util/test_util.h
@@ -78,6 +78,9 @@ class KuduTest : public ::testing::Test {
std::string* key, std::string* iv, std::string*
version);
protected:
+ // Sets the flags to enable encryption if 'enable_encryption' is true.
+ static void SetEncryptionFlags(bool enable_encryption);
+
// Returns absolute path based on a unit test-specific work directory, given
// a relative path. Useful for writing test files that should be deleted
after
// the test ends.
@@ -89,9 +92,6 @@ class KuduTest : public ::testing::Test {
// (and the flags reset) before test_dir_ is deleted.
std::unique_ptr<google::FlagSaver> flag_saver_;
- // Sets the flags to enable encryption if 'enable_encryption' is true.
- void SetEncryptionFlags(bool enable_encryption);
-
std::string test_dir_;
};
