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 10 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::getenvand see if the result is correct - Override 
std::getenvadding 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 10 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 10 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 10 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 10 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 10 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 10 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 9 years ago
      
    
    - Assignee set to Alex Afanasyev
 - % Done changed from 0 to 100
 
      
      Updated by Alex Afanasyev over 9 years ago
      
    
    - Status changed from New to Code review
 
      
      Updated by Alex Afanasyev over 9 years ago
      
    
    - Status changed from Code review to Closed