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

Various Path FFI improvements #269

Merged
merged 5 commits into from Dec 5, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Dec 5, 2017

See commit messages for details.

The first is effectively not changing anything other than adding comments in the code, using expect() instead of unwrap() with more details, and not lossy converting non-UTF8 paths from GLib to Rust on Windows but fail instead on invalid UTF-8 (otherwise we might access the wrong file! potential data loss, etc, and it's a bug in the C library if that ever happens)

sdroege added some commits Dec 5, 2017

Fix/improve the translation from GLib paths to Rust paths and back
First of all, add comments in the Windows and UNIX specific cases about
what a GLib path is, and what a Rust path is.

On UNIX, paths are in the local encoding in GLib and in Rust and must
never contain NUL bytes except for the final NUL terminator. As such we
can safely panic if this is ever not true, there's nothing else we can
do in this case and the previous code already implicitly did that by
unwrapping.

On Windows, GLib paths are always UTF-8 and Rust paths are WTF-8,
which is a superset of UTF-8 and not every WTF-8 string can be converted
back to UTF-8. As such, conversion from Rust to GLib can fail and we
can't come up with any UTF-8 string that would represent the same path
and have to panic (and did so implicitly by unwrapping before already).
Conversion from GLib to Rust can't possibly fail, unless there is a bug
in the C library and the string is not valid UTF-8. In this case,
instead of replacing the invalid characters with the replacement
character, the best we can do is to panic. By using the replacement
characters, in the worst case a different (and existing!) path could be
used, resulting in undefined behaviour and potential data loss.
Handle "long path" paths starting with "\\?\" on Windows correctly
On Windows, paths can have \\?\ prepended for long-path support. See
the MSDN documentation about CreateFile, for example

We have to get rid of this and let GLib take care of all these
weirdnesses later
@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

Please carefully review, @EPashkin and @GuillaumeGomez :)

The second commit is also untested on Windows, if someone can give it some testing... you can get the annoying paths by using Path::canonicalize().

//
// We have to get rid of this and let GLib take care of all these
// weirdnesses later
if path_str.starts_with("\\\\?\\") {

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 5, 2017

Member

Still makes me laugh so hard.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Seems good to me, just a bit sad for all the new panics... Otherwise, huge 👍 for the very complete comments, tt helps a lot for understanding!

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

Seems good to me, just a bit sad for all the new panics

There is only one new panic: when converting a non-UTF8 path from GLib to Rust on Windows (which would previously access the same path with the non-UTF8 characters replaced by �, and thus potentially accessing a completely different file). All other panics were there before already, just that they were a simple unwrap at a later time :)

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Ah indeed, my bad! Then huge 💯 for switching from unwrap to expect!

use std::os::unix::ffi::OsStrExt;
CString::new(path.as_os_str().as_bytes()).ok()
CString::new(path.as_os_str().as_bytes())
.expect("Invalid path with NUL bytes")
}
#[cfg(not(unix))]

This comment has been minimized.

@EPashkin

EPashkin Dec 5, 2017

Member

Maybe this need changed to сfg(windows) and previous to cfg(not(windows)) ?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 5, 2017

Member

Oh true! And the #[cfg(unix)] should be changed to #[cfg(not(windows))].

This comment has been minimized.

@sdroege

sdroege Dec 5, 2017

Member

Yeah I was wondering about that. It's more like windows and not-windows instead of unix and not-unix.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Dec 5, 2017

Member

Then we all agree haha.

This comment has been minimized.

@sdroege
Use cfg(windows) and cfg(not(windows)) for Windows-specific code
It's not UNIX vs not-UNIX, but Windows vs. not-Windows.
@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

The second commit is also untested on Windows, if someone can give it some testing... you can get the annoying paths by using Path::canonicalize().

Works fine on Windows

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Question: should we add tests for these functions? I think it might be a good idea (even more since it's testable!).

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

Question: should we add tests for these functions? I think it might be a good idea (even more since it's testable!).

I was wondering about that, but is there a proper way of doing things with files during tests? How/where could we create random files and use them during the test?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

There are great crates for that like tempdir (then you create your files in the temporary directory).

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

How would I add that as a dependency only for tests? dev-dependencies?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

dev-dependencies indeed. :)

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

Ok, writing something

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

There

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Thanks!

@EPashkin: We don't have a cargo test ran in travis nor appveyor?

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 5, 2017

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

@sdroege Can you add a cargo test in .travis.yml please?

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

Sure, done

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Thanks! Just waiting for CIs now and then I merge (unless you have something @EPashkin?).

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 5, 2017

@GuillaumeGomez All looks good and reasonable.
@sdroege Thanks

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 5, 2017

There many GLib-GObject-CRITICAL **: g_value_unset: assertion 'G_IS_VALUE (value)' failed in travis log

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

From another test though, but I also can't reproduce it here. All clean here

Can you reproduce?

@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 5, 2017

I'm not: it on linux.
IMHO better just merge as warning seems not related to this PR and can be fixed later.

@sdroege

This comment has been minimized.

Member

sdroege commented Dec 5, 2017

I don't get these on Linux either, that's where I'm usually testing.

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Dec 5, 2017

Then I merge. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 4e735da into gtk-rs:master Dec 5, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@EPashkin

This comment has been minimized.

Member

EPashkin commented Dec 5, 2017

Errors don't repeated on Ubuntu 16.04 LTS too.

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