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

fix: Correctly create directories for paths with forward slashes on Windows #266

Merged
merged 3 commits into from
Jun 8, 2020

Conversation

Mixaill
Copy link
Contributor

@Mixaill Mixaill commented May 31, 2020

Fixes sentry-native initialization on Windows when databasepath has forward slashes.

@jan-auer jan-auer requested a review from Swatinem June 2, 2020 07:49
Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! We should have a unit test for this.

src/path/sentry_path_windows.c Outdated Show resolved Hide resolved
@jan-auer jan-auer changed the title fix: correctly create directories for paths with forward slashes on Windows fix: Correctly create directories for paths with forward slashes on Windows Jun 2, 2020
@Mixaill Mixaill force-pushed the windows-forward-session branch from a56cab6 to 678e4c3 Compare June 3, 2020 16:57
@Mixaill
Copy link
Contributor Author

Mixaill commented Jun 3, 2020

@Swatinem updated

@Swatinem
Copy link
Member

Swatinem commented Jun 3, 2020

Having this tested would be nice. I think its enough to just add a /qux or something here:

sentry_path_t *nested2 = sentry__path_join_str(nested, "baz");

@Mixaill
Copy link
Contributor Author

Mixaill commented Jun 3, 2020

@Swatinem tests added.

I do not known, if backslashes should be tested on non-windows platforms too?

update: it works.

@Mixaill Mixaill force-pushed the windows-forward-session branch 2 times, most recently from 66525f5 to f3da259 Compare June 3, 2020 19:08
@Swatinem
Copy link
Member

Swatinem commented Jun 4, 2020

Sorry for the back and forth here…

Unices are apparently very forgiving when it comes to paths, but you do have an error in there.

/home/vsts/work/1/s/tests/unit/test_path.c:150:46: warning: incompatible pointer types passing 'int [4]' to parameter of type 'sentry_pathchar_t *' (aka 'char *') [-Wincompatible-pointer-types]
    sentry_path_t *path_1 = sentry__path_new(L"foo");
                                             ^~~~~~
/home/vsts/work/1/s/src/sentry_path.h:220:37: note: passing argument to parameter 's' here
sentry__path_new(sentry_pathchar_t *s)
                                    ^

sentry__path_new uses a different typedef for the chars per platform. I think a better test is to compare the behavior of sentry__path_from_str with calling sentry__path_join_str directly, and see that they match up.

@Mixaill Mixaill force-pushed the windows-forward-session branch from f3da259 to a87f4ef Compare June 4, 2020 08:19
@Mixaill
Copy link
Contributor Author

Mixaill commented Jun 4, 2020

sentry__path_new uses a different typedef for the chars per platform

fixed

I think a better test is to compare the behavior of sentry__path_from_str with calling sentry__path_join_str directly, and see that they match up.

My point that sentry_path_windows.c::sentry__path_create_dir_all() does not call these functions at all. So it maybe worth to add such kind of test, but it is a bit out of scope for this PR.

@Swatinem
Copy link
Member

Swatinem commented Jun 4, 2020

The reason I would like to compare to manually calling sentry__path_join_str is that I think the testcase is buggy on unix right now.

if (*ptr == '/' && ptr != p) {

I think unixes will just happily create a directory named "foo\bar", which is different from "foo" / "bar".

@Swatinem
Copy link
Member

Swatinem commented Jun 4, 2020

Which reminds me… should we maybe normalize separators on unix as well? How common is it to use \ as separator on unix? Maybe @mitsuhiko can comment here, since you know a lot about path encodings.

@Mixaill
Copy link
Contributor Author

Mixaill commented Jun 4, 2020

I think the testcase is buggy on unix right now.

Thanks, now I understood it. Yeah, it is definitely broken, because \ is allowed character on unix.

How common is it to use \ as separator on unix?

At least it's possible that someone actually uses the backslash as a folder name in unix --> it will break sentry-native for him.

The reason I would like to compare to manually calling sentry__path_join_str

It will be failed, because sentry__path_from_str does not perform normalization. Maybe we should add it at least for Windows?

@mitsuhiko
Copy link
Member

We should only execute tests with \\ as separators on windows and skip this on Linux. We should not convert backslashes to forward slashes on UNIX. That's unexpected and may cause issues if someone actually wants to use them.

@Mixaill Mixaill force-pushed the windows-forward-session branch from a87f4ef to c1f5869 Compare June 8, 2020 09:22
@Mixaill
Copy link
Contributor Author

Mixaill commented Jun 8, 2020

We should only execute tests with \ as separators on windows and skip this on Linux.

Done

@Swatinem Swatinem merged commit 621ee38 into getsentry:master Jun 8, 2020
@Mixaill Mixaill deleted the windows-forward-session branch June 11, 2020 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants