Skip to content

edmWriteConfigs: don't use unsecure tmpnam(...) #44314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
iarspider wants to merge 6 commits into from
Closed

edmWriteConfigs: don't use unsecure tmpnam(...) #44314

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7531a10
edmWriteConfigs: don't use unsecure tmpnam(...)
iarspider Mar 5, 2024
a6a68a1
Code-format
iarspider Mar 5, 2024
9338b20
Update FWCore/ParameterSet/bin/edmWriteConfigs.cpp
iarspider Mar 5, 2024
bcc7dae
Update FWCore/ParameterSet/bin/edmWriteConfigs.cpp
iarspider Mar 5, 2024
a97d49f
Update edmWriteConfigs.cpp
iarspider Mar 5, 2024
b2d6d37
Code-format
iarspider Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 32 additions & 11 deletions FWCore/ParameterSet/bin/edmWriteConfigs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
#include <sstream>
#include <fstream>
#include <filesystem>
#include <cstdlib>
#include <unistd.h>

static char const* const kHelpOpt = "help";
static char const* const kHelpCommandOpt = "help,h";
Expand Down Expand Up @@ -136,18 +138,33 @@ namespace {
}
}

void writeModulesFile() {
bool open_temp(std::string& path, std::ofstream& f) {
path += "/XXXXXX";
path += '\0'; // ensure that the string is null-terminated
int fd = mkstemp(path.data());
if (fd != -1) {
f.open(path.c_str(), std::ios_base::trunc | std::ios_base::out);
close(fd);
return true;
}
return false;
}
Comment on lines +141 to +151
Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to separate the generation of a unique temporary file name and opening of the file. It would also be better to use the exceptions for error handling (since the caller of writeModulesFile() already has an exception handler).

Also, from C++11 onwards the std::string::data() is guaranteed to be null-terminated (cppreference)

Please change along

Suggested change
bool open_temp(std::string& path, std::ofstream& f) {
path += "/XXXXXX";
path += '\0'; // ensure that the string is null-terminated
int fd = mkstemp(path.data());
if (fd != -1) {
f.open(path.c_str(), std::ios_base::trunc | std::ios_base::out);
close(fd);
return true;
}
return false;
}
std::string makeUniqueFilename(std::string path) {
path += "/XXXXXX";
int fd = mkstemp(path.data());
if (fd == -1) {
throw cms::Exception("Failed unique file name") << "Unable to create a unique file name with the template " << path << ": " << std::strerror(errno);
}
close(fd);
return path;
}

Copy link
Contributor

@fwyzard fwyzard Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach (using a call to generate the file name, and a separate call to open the file) is not safe: that's the whole point of not using tmpnam() in the first place.

Copy link
Contributor Author

@iarspider iarspider Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use low-level function write instead of C++ streams?

Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach (using a call to generate the file name, and a separate call to open the file) is not safe

Why not? mkstemp() documentation has

The characters shall be chosen such that the resulting pathname does not duplicate the name of an existing file at the time of the call to mkstemp()

so the file name should be unique among other processes (which is better than the behavior of tmpnam()

it is possible that a file with that name is created by another process between the moment std::tmpnam returns and the moment this program attempts to use the returned name to create a file.

).

I agree it is possible that something else could tamper with the created file between the close(fd) and the ofstream construction. If this something else would be by us, we can change it (and currently there should be nothing else doing that). If this something else would be a malicious third party, I think we'd have bigger problems elsewhere.

Copy link
Contributor

@fwyzard fwyzard Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is exactly the behaviour of tmpnam.

With std::tmpnam():

  • create a unique filename that does not name a currently existing file, and stores it in the character string pointed to by filename
  • it is possible that a file with that name is created by another process between the moment std::tmpnam returns and the moment this program attempts to use the returned name to create a file.

With mkstemp() closing the file and recreating

  • the mkstemp() function generates a unique temporary filename from template, creates and opens the file, and returns an open file descriptor for the file.
  • if we close the file descriptor and open a new file, it's possible that another process creates a file with that same name in the meantime.

So replacing tmpnam with mkstemp, closing the file and re-opening, we do not gain absolutely anything.

Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there can be a difference between tmpnam() and mkstemp(). With two or more processes using tmpnam(), I'd expect it to be possible two (or more) of the processes to get the same value from their tmpnam() calls. When all the processes use mkstemp(), this would not happen as all the processes would be guaranteed to get different names (as long as the temporary files were kept around), or the mkstemp() call would fail.

Written that, given that the best solution would be to move the generation of modules.py to scram (#44314 (comment)), which could hopefully be done at the timescale of a few weeks, it doesn't seem worth to spend too much time in improving the behavior, even with respect to tmpnam().

Copy link
Contributor

@fwyzard fwyzard Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... yes, I see what you mean.


bool writeModulesFile() {
Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With exceptions the function signature can return to

Suggested change
bool writeModulesFile() {
void writeModulesFile() {

if (std::filesystem::exists(std::filesystem::current_path() / "modules.py"))
return;
std::array<char, L_tmpnam> buffer;
std::tmpnam(buffer.data());
std::ofstream file{buffer.data()};

file << "from FWCore.ParameterSet.ModulesProxy import _setupProxies\n"
"locals().update(_setupProxies(__file__))\n";
file.close();
std::filesystem::copy(buffer.data(), "modules.py");
std::filesystem::remove(buffer.data());
std::ofstream file;
std::string path(std::filesystem::current_path());
bool res = open_temp(path, file);
if (res) {
file << "from FWCore.ParameterSet.ModulesProxy import _setupProxies\n"
"locals().update(_setupProxies(__file__))\n";
file.close();
std::filesystem::copy(path.data(), "modules.py");
std::filesystem::remove(path.data());
return true;
}
return false;
Comment on lines +156 to +167
Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this could become

Suggested change
std::ofstream file;
std::string path(std::filesystem::current_path());
bool res = open_temp(path, file);
if (res) {
file << "from FWCore.ParameterSet.ModulesProxy import _setupProxies\n"
"locals().update(_setupProxies(__file__))\n";
file.close();
std::filesystem::copy(path.data(), "modules.py");
std::filesystem::remove(path.data());
return true;
}
return false;
auto const path = makeUniqueFilename(std::filesystem::current_path());
std::ofstream file{path.c_str());
file << "from FWCore.ParameterSet.ModulesProxy import _setupProxies\n"
"locals().update(_setupProxies(__file__))\n";
file.close();
std::filesystem::copy(path.c_str(), "modules.py");
std::filesystem::remove(path.c_str());

}
} // namespace

Expand Down Expand Up @@ -304,7 +321,11 @@ int main(int argc, char** argv) try {
std::set<std::string> usedCfiFileNames;
edm::for_all(pluginNames, std::bind(&writeCfisForPlugin, _1, factory, std::ref(usedCfiFileNames)));
if (not pluginNames.empty()) {
writeModulesFile();
bool res = writeModulesFile();
if (!res) {
std::cerr << "writeModulesFile() failed" << std::endl;
return 1;
}
Comment on lines +324 to +328
Copy link
Contributor

@makortel makortel Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this would go back to

Suggested change
bool res = writeModulesFile();
if (!res) {
std::cerr << "writeModulesFile() failed" << std::endl;
return 1;
}
writeModulesFile();

}
});
} catch (cms::Exception& iException) {
Expand Down