replaced mktemp with tempfile for supress warning mktemp is dangerous #198

akoserwal · 2018-05-20T05:12:13Z

No description provided.

akoserwal · 2018-05-20T05:12:42Z

dlrobertson · 2018-05-22T02:06:20Z

src/platform/unix/mod.rs

+                //avoid deleting temp director automatically
+                let _tmp_path = dir.into_path();
+
+                let mut _tmp_file = tempfile_in(&_tmp_path).unwrap();


Are _tmp_path and _tmp_file needed? also dir doesn't live long enough. TempDir implements Drop, so it will be closed at the end of this function.

yes, test run got stuck. If I remove _tmp_path and _tmp_file. tempdir is getting dropped before socket bind operation.

test platform::test::sender_transfer ... ok thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: UnixError(2)', libcore/result.rs:945:5 note: Run with `RUST_BACKTRACE=1` for a backtrace. test platform::test::cross_process ... test platform::test::cross_process has been running for over 60 seconds test platform::test::cross_process_sender_transfer ... test platform::test::cross_process_sender_transfer has been running for over 60 seconds

Same case when I remove _tmp_path. and use either File::create or tempfile_in using tempDir path.

let mut _tmp_file = tempfile_in(&dir).unwrap(); ```

If I remove _tmp_path and _tmp_file. tempdir is getting dropped before socket bind operation.

Can't you add dir to the OsIpcOneShotServer so it doesn't get dropped.

dlrobertson · 2018-05-22T19:51:10Z

src/platform/unix/mod.rs

@@ -31,6 +31,8 @@ use std::time::UNIX_EPOCH;
 use std::thread;
 use mio::unix::EventedFd;
 use mio::{Poll, Token, Events, Ready, PollOpt};
+use tempfile::{TempDir};


no need for braces or empty line afterwards.

dlrobertson · 2018-05-22T19:51:10Z

src/platform/unix/mod.rs

@@ -577,6 +579,7 @@ impl OsOpaqueIpcChannel {

 pub struct OsIpcOneShotServer {
    fd: c_int,
+    _dir:TempDir,


might want to add a comment as to why this is needed.

antrik · 2018-05-23T03:53:18Z

@akoserwal you seem to have two PRs open for the same thing? Unless I'm missing something, please close one of them...

antrik

To be honest, the approach using a dedicated directory seems a bit circumspect to me, as opposed to simply generating a more unique/unguessable name, using something like libc::tmpnam()...

(Are symlink attacks even possible against sockets?)

On the other hand, this approach allows using the existing crate, which also takes care of cleanup (otherwise we would have to add manual handling for that: the original implementation doesn't do it at all!) -- so all in all it's a good thing I guess :-)

I found a couple of issues with your code though...

BTW, you should probably put closes https://github.com/servo/ipc-channel/issues/11 somewhere in your commit message, so GitHub will automatically close the issue when the pull request is merged.

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -31,6 +31,8 @@ use std::time::UNIX_EPOCH;
 use std::thread;
 use mio::unix::EventedFd;
 use mio::{Poll, Token, Events, Ready, PollOpt};
+use tempfile::TempDir;
+



Remove the spurious extra blank line you added here.

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -575,8 +577,11 @@ impl OsOpaqueIpcChannel {
    }
 }

+//_dir: keeps track of temp directory required when the socket is created and drops temp directory automatically.
+


The comment should go directly in front of the field it pertains to, not in front of the containing structure. (Maybe separated from the previous field by a blank line, but with no blank line between the comment and the field it pertains to.)

Also, the comment is a bit unclear IMHO... I'd suggest something like:

// Object representing the temporary directory the socket was created in. // The directory is automatically deleted (along with the socket inside it) // when this field is dropped.

Might also be good to rename the field itself to _temp_dir?

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

 pub struct OsIpcOneShotServer {
    fd: c_int,
+    _dir:TempDir,


Missing a blank after the :.

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -593,13 +598,15 @@ impl OsIpcOneShotServer {
        unsafe {
            let fd = libc::socket(libc::AF_UNIX, SOCK_SEQPACKET, 0);
            let mut path: Vec<u8>;
+            let dir = TempDir::new().unwrap();
+            let dir_path = dir.path().join("rust-ipc-socket");


dir_path is a confusing name: it sounds like it was the path of the temporary directory... Rename it to something like socket_path I'd say -- or maybe just path? (Note that the other path variable should be gone once you address my remark about back-and-forth conversion below...)

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -593,13 +598,15 @@ impl OsIpcOneShotServer {
        unsafe {
            let fd = libc::socket(libc::AF_UNIX, SOCK_SEQPACKET, 0);
            let mut path: Vec<u8>;
+            let dir = TempDir::new().unwrap();
+            let dir_path = dir.path().join("rust-ipc-socket");
+


This blank line has trailing white space. Git should have warned you about that while committing...

Still has the trailing white space...

You can see the extra white space in the GitHub diff if you select the line; and locally, git diff marks this very visibly. (Unless diff colouring is disabled for some reason...)

Ideally, you should configure your text editor to show trailing white space, so you can avoid such mistakes in the first place.

That said, I don't think the blank line here is actually useful any more after the latest changes...

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

+                let path_string = CString::new(dir_path.to_str().unwrap()).unwrap();
+                path = path_string.as_bytes_with_nul().iter().cloned().collect();
+
+


More trailing white space; and (at least) one blank line too much...

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs


+                let path_string = CString::new(dir_path.to_str().unwrap()).unwrap();
+                path = path_string.as_bytes_with_nul().iter().cloned().collect();


The complex cloning and back-and-forth conversion was only necessary because mktemp() mutates the string. You can use a much simpler approach now, like in connect().

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -616,7 +623,7 @@ impl OsIpcOneShotServer {
            }

            Ok((OsIpcOneShotServer {
-                fd: fd,
+                fd: fd, _dir: dir,


The second field initialiser goes on a separate line.

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -593,13 +598,15 @@ impl OsIpcOneShotServer {
        unsafe {
            let fd = libc::socket(libc::AF_UNIX, SOCK_SEQPACKET, 0);
            let mut path: Vec<u8>;
+            let dir = TempDir::new().unwrap();


Please use tempfile::Builder (https://docs.rs/tempfile/3.0.2/tempfile/struct.Builder.html), to get a meaningful directory name. (The actual file name can then be shortened to just "socket" I guess.)

antrik · 2018-05-23T04:28:36Z

src/platform/unix/mod.rs

@@ -593,13 +598,15 @@ impl OsIpcOneShotServer {
        unsafe {
            let fd = libc::socket(libc::AF_UNIX, SOCK_SEQPACKET, 0);
            let mut path: Vec<u8>;
+            let dir = TempDir::new().unwrap();
+            let dir_path = dir.path().join("rust-ipc-socket");
+
            loop {


With the new approach, you can drop the loop and the EINVAL handling I believe: AIUI we only needed to retry, because using mktemp() is not atomic. (Someone else might have taken the name between our mktemp() and bind() calls.) tempfile doesn't have that issue.

Might be prudent though to do this change in a separate commit, just in case... (While all your other changes of course will have to be squashed into one before this can be merged.)

antrik · 2018-05-23T21:09:25Z

src/platform/unix/mod.rs

-            let dir_path = dir.path().join("rust-ipc-socket");
+            let _temp_dir =  Builder::new()
+                .tempdir()
+                .unwrap();


A few nitpicks here: the local temp_dir variable doesn't need to start with _, since it's actually used. (Unlike the _temp_dir field in OsIpcOneShotServer.)

There should be only one blank after the =. (You have two...)

The chained calls easily fit on one line (and pose no readability issues) -- no need to use extra lines for them.

antrik · 2018-05-23T21:09:25Z

src/platform/unix/mod.rs

@@ -31,8 +31,7 @@ use std::time::UNIX_EPOCH;
 use std::thread;
 use mio::unix::EventedFd;
 use mio::{Poll, Token, Events, Ready, PollOpt};
-use tempfile::TempDir;
-
+use tempfile::{Builder,TempDir};


Please add a blank after the ,.

Sorry if these formatting issues sound pedantic -- but it's important to stick with the conventions, as otherwise people will always stumble while reading the code...

@antrik: No problem, it's imp & I appreciate it. thanks

antrik · 2018-05-23T21:09:25Z

src/platform/unix/mod.rs

@@ -593,13 +598,15 @@ impl OsIpcOneShotServer {
        unsafe {
            let fd = libc::socket(libc::AF_UNIX, SOCK_SEQPACKET, 0);
            let mut path: Vec<u8>;
+            let dir = TempDir::new().unwrap();
+            let dir_path = dir.path().join("rust-ipc-socket");
+


Still has the trailing white space...

You can see the extra white space in the GitHub diff if you select the line; and locally, git diff marks this very visibly. (Unless diff colouring is disabled for some reason...)

Ideally, you should configure your text editor to show trailing white space, so you can avoid such mistakes in the first place.

That said, I don't think the blank line here is actually useful any more after the latest changes...

antrik · 2018-05-23T21:09:25Z

src/platform/unix/mod.rs

-            }, String::from_utf8(CStr::from_ptr(path.as_ptr() as
+                fd: fd,
+                _temp_dir: _temp_dir,
+            }, String::from_utf8(CStr::from_ptr(path_string.as_ptr() as
                                                *const c_char).to_bytes().to_owned()).unwrap()))


This complicated conversion is not necessary any more: you actually already have the final Rust string after the socket_path.to_str().unwrap() (before converting it to CString) -- you just need to add a .to_owned(), and save it in another temporary variable. (Or even chain this conversion right in the assignment of socket_path.)

antrik · 2018-05-24T11:00:34Z

src/platform/unix/mod.rs

-                }
+            let temp_dir =  Builder::new().tempdir().unwrap();
+            let socket_path = temp_dir.path().join("socket");
+            let path_string = socket_path.to_str().to_owned().unwrap();


to_owned() in this place is useless. My suggestion was to add it after the unwrap(), to convert the resulting &str into a String. (Maybe I should have said to_string() instead to make it clearer... Not sure whether there is consensus nowadays which one is preferable.)

Though doing the to_string() conversion only while returning the result, as you did, is fine too -- i.e. you don't need the extra to_owned() or to_string() here at all.

So, it will be like

let path_string = CString::new(socket_path.to_str().unwrap()).unwrap();

No, that's what you had before... Your last version was actually closer.

Sorry if I'm not making myself clear. I was thinking of something like

let name = socket_path.to_str().unwrap(); let name_ffi = CString::new(name).unwrap();

(And later return name.to_string().)

Maybe you could even do the CString conversion directly in the new_sockaddr_un() call, since you do not need the value of name_ffi anywhere else...

@antrik : thank you so much for keeping patience with me.

antrik · 2018-05-24T11:00:34Z

src/platform/unix/mod.rs

-                if errno.0 != libc::EINVAL {
-                    return Err(errno)
-                }
+            let (sockaddr, len) = new_sockaddr_un(path_string.as_ptr() as *const c_char);


There was a misunderstanding here: you still do need to convert the string to a CString before passing it to new_sockaddr_un. My point was just that you can grab the string to be returned at the end before doing the conversion to CString, rather than converting the CString back to a standard String...

antrik · 2018-05-24T11:03:24Z

BTW, I said to add closes https://github.com/servo/ipc-channel/issues/11, rather than just closes #11, for a reason: while it looks the same in the GitHub interface, it's not the same when looking at the commit message through other means...

src/platform/unix/mod.rs

antrik · 2018-05-27T17:08:06Z

src/platform/unix/mod.rs

-                if libc::bind(fd, &sockaddr as *const _ as *const sockaddr, len as socklen_t) == 0 {
-                    break
-                }
+            let temp_dir =  Builder::new().tempdir().unwrap();


I just noticed there is a second blank after the = here as well...

antrik · 2018-05-27T17:08:06Z

src/platform/unix/mod.rs

@@ -617,7 +613,8 @@ impl OsIpcOneShotServer {

            Ok((OsIpcOneShotServer {
                fd: fd,
-            }, String::from_utf8(CStr::from_ptr(path.as_ptr() as
+                _temp_dir: temp_dir,
+            }, String::from_utf8(CStr::from_ptr(path_string.as_ptr() as
                                                *const c_char).to_bytes().to_owned()).unwrap()))


You didn't remove the back-and-forth conversion here -- but that was the whole point of shuffling around the other conversions/bindings above...

So, this will be just
path_string.to_string()
right?

antrik · 2018-05-28T17:42:01Z

Looks good now :-)

Please squash the commits into one, and give it a nice commit message, describing what you did and why.

(The title line could read something like "unix: Use tempfile crate for creating socket location". How much you write in the long description, depends on how ambitious you feel about this -- I for my part would mention getting rid of mktemp() for security concerns and the associated linker warning; briefly explain how we instead safely create an exclusive temporary directory to put the socket in; and how we do not need the loop any more. Not forgetting to point out that as an extra bonus, this takes care of cleanup, since tempdir does that automatically... And of course closing the issue.)

Just in case you are unsure how to a good commit message could look like, I suggest reading through a bunch of existing commit messages for inspiration :-)


        unix: Use tempfile crate for creating socket location

Removed mktemp() for creating the temporary location for the socket creation. Addressing the security concern of easy guessable name & race condition required the loop for checking whether the file exists or not. Used tempfile crate, which provides builder pattern for safely creating an exclusive temporary location and take cares of dropping it automatically along with socket in it. closes #11

antrik · 2018-05-29T18:36:22Z

Oh, you already updated it :-)

(I didn't see it earlier, since GitHub doesn't notify on rebases...)

@bors-servo r+

bors-servo · 2018-05-29T18:36:23Z

📌 Commit 8d72a95 has been approved by antrik

bors-servo · 2018-05-29T18:36:26Z

⌛ Testing commit 8d72a95 with merge f2723d0...


        Auto merge of #198 - akoserwal:fix-tempdir-repace-mktemp, r=antrik

replaced mktemp with tempfile for supress warning mktemp is dangerous

bors-servo · 2018-05-29T18:49:48Z

💔 Test failed - status-travis

bors-servo · 2018-05-29T18:56:26Z

☀️ Test successful - status-appveyor, status-travis
Approved by: antrik
Pushing f2723d0 to master...

akoserwal · 2018-05-29T19:19:09Z

@antrik @dlrobertson : thank you, great learning experience :)

akoserwal mentioned this pull request May 20, 2018

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

Closed

dlrobertson reviewed May 22, 2018

View changes

highfive added the S-awaiting-review label May 22, 2018

dlrobertson reviewed May 22, 2018

View changes

antrik reviewed May 23, 2018

View changes

antrik reviewed May 24, 2018

View changes

akoserwal reviewed May 24, 2018

View changes

src/platform/unix/mod.rs Show resolved Hide resolved

antrik reviewed May 27, 2018

View changes

akoserwal force-pushed the akoserwal:fix-tempdir-repace-mktemp branch from 556d25a to 8d72a95 May 29, 2018

highfive removed the S-awaiting-review label May 29, 2018

highfive added the S-awaiting-merge label May 29, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels May 29, 2018

bors-servo merged commit 8d72a95 into servo:master May 29, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

		let path_string = CString::new(dir_path.to_str().unwrap()).unwrap();
		path = path_string.as_bytes_with_nul().iter().cloned().collect();

servo / ipc-channel

Join GitHub today

replaced mktemp with tempfile for supress warning mktemp is dangerous #198

replaced mktemp with tempfile for supress warning mktemp is dangerous #198

Conversation

akoserwal commented May 20, 2018

akoserwal commented May 20, 2018

This comment has been minimized.

dlrobertson May 22, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antrik commented May 23, 2018

antrik left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antrik May 23, 2018 • edited

This comment has been minimized.

This comment has been minimized.

akoserwal May 24, 2018 • edited

This comment has been minimized.

This comment has been minimized.

akoserwal May 27, 2018 • edited

This comment has been minimized.

antrik commented May 24, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

antrik commented May 28, 2018

antrik commented May 29, 2018

bors-servo commented May 29, 2018

bors-servo commented May 29, 2018

bors-servo commented May 29, 2018

bors-servo commented May 29, 2018

akoserwal commented May 29, 2018

dlrobertson May 22, 2018 •

edited

antrik May 23, 2018 •

edited

akoserwal May 24, 2018 •

edited

akoserwal May 27, 2018 •

edited