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

Create temporary path with boost::filesystem for cross platform compatibility #1063

Closed
wants to merge 3 commits into from
Closed

Create temporary path with boost::filesystem for cross platform compatibility #1063

wants to merge 3 commits into from

Conversation

kloudkl
Copy link
Contributor

@kloudkl kloudkl commented Sep 10, 2014

No description provided.

@kloudkl
Copy link
Contributor Author

kloudkl commented Sep 11, 2014

@jeffdonahue, what's your opinion?

@Yangqing
Copy link
Member

LGTM.

@netheril96
Copy link
Contributor

Both the old and new version have a potential race condition, because determining the temporary file name and opening it (the old one closes the file descriptor returned by mkstemp) is separate. This introduces the possibility that when the program opens the temporary file, it already exists and may either fail because of insufficient permissions, or cause security problems. The API of mkstemp is specifically designed to avoid this problem, an approach that should be adopted here. That is, instead of create a temporary file/directory name, just return a file descriptor or FILE*/DIR*.

@kloudkl
Copy link
Contributor Author

kloudkl commented Oct 13, 2014

@netheril96, Would you like to finish the task?

@netheril96
Copy link
Contributor

@kloudkl Except that there is no easy way to implement the functionality of mkstemp in a cross platform manner. Neither boost or windows api exposes a similar function.

There sure is a way, given that Python has a mkstemp that is undoubtedly based on system API. But I don't know what it is.

@shelhamer
Copy link
Member

Closing since there's still a race condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants