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

update mktemp to mkdtemp to supress warning mktemp is dangerous #196

Closed
wants to merge 2 commits into from

Conversation

@akoserwal
Copy link

akoserwal commented May 13, 2018

warning: the use of mktemp' is dangerous, better use mkstemp' or `mkdtemp'
#11

@highfive
Copy link
Collaborator

highfive commented May 13, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

@@ -596,7 +596,7 @@ impl OsIpcOneShotServer {
loop {
let path_string = CString::new(&b"/tmp/rust-ipc-socket.XXXXXX"[..]).unwrap();
path = path_string.as_bytes_with_nul().iter().cloned().collect();
if *mktemp(path.as_mut_ptr() as *mut c_char) == 0 {
if *mkdtemp(path.as_mut_ptr() as *mut c_char) == 0 {

This comment has been minimized.

@dlrobertson

dlrobertson May 13, 2018

Collaborator

mkdtemp creates a directory right? Wouldn't that cause problems with the subsequent new_sockaddr_un call?

This comment has been minimized.

@akoserwal

akoserwal May 13, 2018

Author

@dlrobertson: right, it should be mkstemp ?

This comment has been minimized.

@dlrobertson

dlrobertson May 13, 2018

Collaborator

We're using mkstemp to basically generate a random path. I'm not sure that mkstemp will work either since it actually opens the file. I think we have a few options.

  1. Create the path some other way (e.g. with a UUID).
  2. use mkdtemp and then use some path on top of that.

This comment has been minimized.

@akoserwal

akoserwal May 14, 2018

Author

@dlrobertson : I think, even with option 1 or 2. problem will be with creating the file. As If we using https://doc.rust-lang.org/std/fs/struct.File.html#method.create. it will be in open mode as mkstemp. I am new to rust/system programming. So, I might be having not clear understanding about the entire scenario.

This comment has been minimized.

This comment has been minimized.

@akoserwal

akoserwal May 20, 2018

Author

@dlrobertson : #198 got it working. all test are passing.

This comment has been minimized.

@antrik

antrik May 23, 2018

Contributor

@dlrobertson you are evil: first you proposed two different valid approaches, but when he implemented one of them, you suddenly ask why he didn't do it another way... ;-)

This comment has been minimized.

@antrik

antrik May 23, 2018

Contributor

@akoserwal you may have figured it out in the meantime; but just to be sure: the issue here is that we do not want to actually create a regular file. Rather, the bind() system call creates a socket special file -- but we need to find a safe location for bind() to put it.

One way to do this is creating a unique and unguessable name up front. (mktemp() is problematic, because the names it creates are not sufficiently unique/unguessable. Your approach using uuid OTOH should have been fine.)

The other way is to safely create a directory for exclusive use (using mkdtemp() or tempfile::TempDir), and then use any name inside that directory for the socket. That's what you are doing in the newer code.

This comment has been minimized.

@dlrobertson

dlrobertson May 23, 2018

Collaborator

@dlrobertson you are evil: first you proposed two different valid approaches, but when he implemented one of them, you suddenly ask why he didn't do it another way... ;-)

lol yeah. I didn't think of using tempfile early enough. Thought it might make the implementation easier.

@akoserwal akoserwal closed this May 23, 2018
@akoserwal akoserwal deleted the akoserwal:master branch May 30, 2018
@akoserwal akoserwal restored the akoserwal:master branch May 30, 2018
@akoserwal akoserwal deleted the akoserwal:master branch Jun 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.