Skip to content
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.

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

Error-prone use of mkstemp() and mkdtemp() in fsutil.cpp #293

Closed
mikebentley15 opened this issue Sep 24, 2019 · 2 comments · Fixed by #304
Closed

Error-prone use of mkstemp() and mkdtemp() in fsutil.cpp #293

mikebentley15 opened this issue Sep 24, 2019 · 2 comments · Fixed by #304
Labels
bug c++ Involves touching c++ code tests Involves touching tests

Comments

@mikebentley15
Copy link
Collaborator

Bug Report

Describe the problem
The calls to mkstemp() and mkdtemp() from within src/fsutil.cpp do not check the return values. These return values are essential to checks for error conditions. For example, if for some unknown reason mkstemp() fails to create a temporary file, we would not want to continue running as if it was created successfully.

Suggested Fix
Check the return values for errors and if an error occurred, throw std::ios_base::failure exceptions.

I would like to create tests for this. Perhaps we would need to use Google Moc? I would like to learn how that works.

Alternative approaches:
Hopefully the problem would be discovered elsewhere in the code. But would we want to risk it?

@mikebentley15 mikebentley15 added bug c++ Involves touching c++ code tests Involves touching tests labels Sep 24, 2019
@IanBriggs
Copy link
Collaborator

I think we should check the status and throw the exception, but I'm not sure if we should add tests for this since it would require another external dependency (though only for testing).

@mikebentley15
Copy link
Collaborator Author

Agreed. If we used Google Mock for this, then we would probably want to move all of our C++ tests to use Google Test (since they are now part of the same framework).

Also, I looked into Google Mock, and it would not help us with this problem unless we changed how we called the functions to begin with (through an interface class that can be used to override the behavior). I don't want to go through that. Perhaps we could do some link seams (see the third answer at https://stackoverflow.com/questions/2924440/advice-on-mocking-system-calls#2924607).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c++ Involves touching c++ code tests Involves touching tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants