Bug #2514
closedUnsafe use of getenv("HOME") in SecPublicInfoSqlite3 and SecTpmFile
100%
Description
Not in all environments, HOME variable is defined. In cases when it is not, the current code results in obscure error. For example, my initial attempt on Android resulted in the following error:
02-12 00:18:08.374: A/NfdWrapper(2378): basic_string::_S_construct null not valid
Updated by Joao Pereira over 9 years ago
I have 3 suggestions to solve this issue
- Use a MACRO that will call the
std::getenv("HOME")
- Add a function in common.hpp or another file that we see feat, to call
std::getenv
and see if the result is correct - Override
std::getenv
adding a default valuenamespace std{ string getenv(string variableName, string defaultValue); }
I like the third option, but not sure if it is nice to be messing around with std functions. Maybe we can do something similar in option 2
Updated by Junxiao Shi over 9 years ago
I disagree with all solutions in note-1.
std::stdenv
shouldn't be altered.
This problem occurs only in SecPublicInfoSqlite3
and SecTpmFile
.
These two classes, after calling std::getenv("HOME")
, should check whether the return value equals nullptr
.
std::getenv("HOME") == nullptr ? "/root" : std::getenv("HOME")
Updated by Joao Pereira over 9 years ago
I thought of centralizing that, because I really do not like to see hardcoded things around, specially paths.
Think that at least the path should be centralized somewhere in a const or something.
const std::string nfd::defaultPath("/root");
and change the code in both places to:
std::getenv("HOME") == nullptr ? nfd::defaultPath : std::getenv("HOME")
Updated by Junxiao Shi over 9 years ago
What about:
std::string
getHomeDirectory();
But this shouldn't appear in common.hpp
because it's not generic enough.
Updated by Joao Pereira over 9 years ago
That was my second solution, but I am not sure where we can put this because it should be accessible in both the places. Is there a .hpp where we store all our generic functions?
Updated by Junxiao Shi over 9 years ago
I looked at util/config-file.cpp
which also uses std::getenv("HOME")
.
I feel it's incorrect to imply "/root"
or any other home directory when HOME
environ is unset.
Therefore, SecPublicInfoSqlite3
and SecTpmFile
could simply report an error when std::getenv("HOME")
equals nullptr.
Updated by Joao Pereira over 9 years ago
What if we add a default value in the config file? It would be more flexible don't you think? We can assume /root as the default value for that parameter
Updated by Alex Afanasyev over 8 years ago
- Assignee set to Alex Afanasyev
- % Done changed from 0 to 100
Updated by Alex Afanasyev over 8 years ago
- Status changed from New to Code review
Updated by Alex Afanasyev over 8 years ago
- Status changed from Code review to Closed