bakaid commented on a change in pull request #731: MINIFICPP-1096 fix
BackTrace, OOB indexing, tests, appveyor reporting
URL: https://github.com/apache/nifi-minifi-cpp/pull/731#discussion_r385254493
##########
File path: libminifi/include/utils/file/FileUtils.h
##########
@@ -112,31 +112,28 @@ class FileUtils {
#endif
}
- static std::string create_temp_directory(char *format) {
+ static std::string create_temp_directory(char * const format) {
#ifdef WIN32
- std::string tempDirectory;
- char tempBuffer[MAX_PATH];
- auto ret = GetTempPath(MAX_PATH, tempBuffer);
- if (ret <= MAX_PATH && ret != 0)
- {
- static std::shared_ptr<minifi::utils::IdGenerator> generator;
- if (!generator) {
- generator =
minifi::utils::IdGenerator::getIdGenerator();
-
generator->initialize(std::make_shared<minifi::Properties>());
- }
- tempDirectory = tempBuffer;
- minifi::utils::Identifier id;
- generator->generate(id);
- tempDirectory += id.to_string();
- create_dir(tempDirectory);
- }
- return tempDirectory;
+ std::string tempDirectory;
+ char tempBuffer[MAX_PATH];
+ auto ret = GetTempPath(MAX_PATH, tempBuffer);
+ if (ret <= MAX_PATH && ret != 0)
+ {
+ static std::shared_ptr<minifi::utils::IdGenerator> generator;
Review comment:
I know this is old code, but it is horrifically unsafe. Since it is
currently only used from tests I don't mind it so much, but:
- `minifi::utils::IdGenerator::getIdGenerator` is already a singleton
getter so this does function static variable does not make much sense aside
from saving a shared_ptr copy
- only the creation (and initialization to nullptr) of `generator` is
protected, the check for nullptr and assignment from
`minifi::utils::IdGenerator::getIdGenerator` is not, so it is a race condition
- `generator->initialize` would not be protected even if `generator` would
be initially initialized to `minifi::utils::IdGenerator::getIdGenerator()`,
that's why `generator->initialize` should only be called at the beginning of a
main executable, when there is only a single thread (see `main/MiNiFiMain.cpp`
and `libminifi/test/TestBase.h`)
This whole part should just be rewritten to
`utils::IdGenerator::getIdGenerator()->generate(id)`, and if it breaks
something, that's the real problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
