OSSL_STORE: Avoid testing with URIs on the command line #4322

levitte · 2017-09-01T21:06:30Z

URIs get mistreated by mingw-built programs in the init code.
Unfortunately, avoiding this conversion doesn't help either.

http://www.mingw.org/wiki/Posix_path_conversion

Fixes #4314

mattcaswell · 2017-09-01T21:37:19Z

test/recipes/90-test_store.t

-            ok(!run(app(["openssl", "storeutl", to_abs_file_uri($file)])));
+        SKIP: {
+                ok(!run(app(["openssl", "storeutl", $file])));
+                ok(!run(app(["openssl", "storeutl", to_abs_file($file)])));


Any particular reason why these (and other similar instances below) are inside the SKIP block?

Estetics. I found it nicer looking of all ok lines were inside SKIP blocks

dot-asm · 2017-09-01T21:47:49Z

Commit message is a bit misleading. The URIs get mistreated by MSYS perl invoking Windows binaries, such as "mingw" ones.

levitte · 2017-09-02T05:23:26Z

@dot-asm, you will have to explain this, then:

Richard@OSFWin7 MINGW32 ~/gitwrk/openssl.org/official/_build
$ ./apps/openssl storeutl file:../master/test/certs
Couldn't open file or uri file;..\master\test\certs
5732:error:02016002:system library:stat:No such file or directory:../master/crypto/store/loader_file.c:812:file;..\master\test\certs

Richard@OSFWin7 MINGW32 ~/gitwrk/openssl.org/official/_build
$ ./apps/openssl storeutl file:/home/Richard/gitwrk/openssl.org/official/master/test/certs
Couldn't open file or uri file;W:\msys32\home\Richard\gitwrk\openssl.org\official\master\test\certs
4344:error:02016002:system library:stat:No such file or directory:../master/crypto/store/loader_file.c:812:file;W:\msys32\home\Richard\gitwrk\openssl.org\official\master\test\certs
4344:error:2C064069:STORE routines:ossl_store_get0_loader_int:unregistered scheme:../master/crypto/store/store_register.c:217:scheme=file;W

No perl in sight in those commands.

However, you're right that it's misleading. Trying it from Windows cmd shows that this isn't happening in the init code of the program:

W:\>cd msys32\home\Richard\gitwrk\openssl.org\official\_build

W:\msys32\home\Richard\gitwrk\openssl.org\official\_build>apps\openssl.exe storeutl file:/home/Richard/gitwrk/openssl.org/official/master/test/certs
Couldn't open file or uri file:/home/Richard/gitwrk/openssl.org/official/master/test/certs
5056:error:02016002:system library:stat:No such file or directory:../master/crypto/store/loader_file.c:812:file:/home/Richard/gitwrk/openssl.org/official/master/test/certs
5056:error:02016002:system library:stat:No such file or directory:../master/crypto/store/loader_file.c:812:/home/Richard/gitwrk/openssl.org/official/master/test/certs

W:\msys32\home\Richard\gitwrk\openssl.org\official\_build>

So it seems it's the mingw shell doing this...

levitte · 2017-09-02T05:26:14Z

I've changed the commit message accordingly. Thanks for pointing this out.

dot-asm · 2017-09-02T08:04:01Z

No perl in sight in those commands.

Right. But in both cases it all boils down to exec in MSYS run-time, which looks at target binary to determine if it's linked with MSYS run-time or not. And if it is, then it just passes argv. And if it isn't, like in case with Windows/mingw binaries, argv gets converted so that target process can make sense of file names. In other words commit message that would reflect situation most accurately would be "Some URIs get "mistreated" (converted) by the MSYS run-time." Reference to "mingw shell" is also misleading, because there is no such thing. Well, depends on definition of "is" in "there is mingw shell". If you look for an icon in Windows start menu, then yes, there is icon called "mingw shell". But it's just MSYS shell, i.e. sh.exe linked with MSYS run-time, with $PATH adjusted so that when you call gcc you end up invoking mingw cross-compiler.

Some URIs get "mistreated" (converted) by the MSYS run-time. Unfortunately, avoiding this conversion doesn't help either. http://www.mingw.org/wiki/Posix_path_conversion Fixes openssl#4314

levitte · 2017-09-02T08:10:23Z

Ok. commit message amended

dot-asm · 2017-09-02T08:27:58Z

avoiding this conversion doesn't help either.

Could you double-check /file:../master/test/certs? I don't have msys2 installed and it takes time [for me] to download it.

dot-asm · 2017-09-02T08:29:24Z

Could you double-check /file:../master/test/certs?

No, don't. I have misinterpreted documentation...

dot-asm · 2017-09-02T08:35:45Z

test/recipes/90-test_store.t


 my $test_name = "test_store";
 setup($test_name);

+# Programs built with mingw have init code that mangles arguments that look


Well, commentary is as misleading as original commit message. Even more so, as you don't really build "with mingw", but rather "for mingw". And of course again, it's not [mingw] init code that mangles things, but MSYS run-time when launching Windows/mingw binaries.

Ah, sorry, forgot that part. Will fix

dot-asm · 2017-09-02T08:44:33Z

test/recipes/90-test_store.t

+# some suitable pattern ("*" to avoid conversions entirely), but that has
+# other unsuitable side effects on paths in the form of URIs, so best avoid
+# them on mingw.
+my $mingw = config('target') =~ m|^mingw|;


Note that this test does pass with mingw cross-compile on Linux. So it's not really config('target') that is the problem, but $^O.

Ah! I forgot that perl on msys had its own platform name... Okie, will fix

$^O

Well, it's combination really. If you were to build openssl with MSYS run-time, the test would pass.

levitte · 2017-09-02T08:46:19Z

Could you double-check /file:../master/test/certs?

I know that you backed down on this, but I wonder, what exactly was that meant to be? From a URI standpoint, it's one with just the path component, /file:../master/test/certs, which would be meaningless, path conversion or no path conversion...

dot-asm · 2017-09-02T08:52:58Z

I know that you backed down on this, but I wonder, what exactly was that meant to be?

The misinterpretation from my side was that / would work as kind of escape char that would "exempt" specific argument.

dot-asm · 2017-09-02T08:59:30Z

test/recipes/90-test_store.t

+                ok(!run(app(["openssl", "storeutl", $file])));
+                ok(!run(app(["openssl", "storeutl", to_abs_file($file)])));
+
+                skip "No test of URI form of $file on mingw", 1 if $msys;


"on msys" or "under msys" perhaps? Multiple occurrences. Though if check is modified for combination of $^O and config('target'), then it would be appropriate to keep reference to mingw, but maybe as [little tongue-in-cheek] "for mingw".

dot-asm · 2017-09-02T09:00:46Z

What's with appveyor?

levitte · 2017-09-02T09:02:56Z

I'm unsure what case checking both $^O and config('target') would cover. Would you mind expanding on that? I'm also unsure when we do and when we don't link with the MSYS run-time...

levitte · 2017-09-02T09:04:50Z

Re appveyor, I have no idea.

dot-asm · 2017-09-02T09:16:57Z

Would you mind expanding on that?

Consider your MSYS2 installation. Is there openssl.exe? As far as I recall there is one. And as far as I recall it's linked with MSYS run-time. Yes, it means that they ought to have local patch that adds msys target. Now, when targeting msys (which is different from "targeting mingw"), all binaries including test ones will be linked with MSYS run-time, and therefore MSYS perl or shell won't get any crazy ideas about mangling command line arguments. And therefore test should pass. To put is it differently check for both $^O and config('target') doesn't make sense for our collection of targets, but our collection of targets is not actually universal. Well, one can always argue that check for $^O is sufficient with rationale that it's not big deal if somebody else's msys target won't exercise ~~this~~ these tests. In other words check for $^O [alone] is good enough, even though it's not really formally complete.

levitte · 2017-09-02T09:30:19Z

That explanation makes sense, thank you. I will fix that later today

levitte · 2017-09-02T09:30:39Z

... I.e fix the check

dot-asm · 2017-09-02T09:35:31Z

... I.e fix the check

As a reminder it might be appropriate to adjust skip informational messages.

levitte · 2017-09-02T15:55:23Z

I ended up rewriting the comment quite a bit. I hope it's close enough to reality.

dot-asm · 2017-09-02T16:23:07Z

test/recipes/90-test_store.t

+#
+#       http://www.mingw.org/wiki/Posix_path_conversion
+#
+# With the built in configurations (all having names starting with "mingw"),


Isn't there dash missing? I mean as in "built-in configurations"?

I think that english allows both forms, but may be wrong. I'll change it.

dot-asm · 2017-09-02T16:24:47Z

test/recipes/90-test_store.t

+# Checking for both msys perl operating environment and that the target name
+# starts with "mingw", we're doing what we can to assure that other configs
+# that might link openssl.exe with the MSYS run-time are not disturbed.
+my $msys = $^O eq 'msys' && config('target') =~ m|^mingw|;


Do you mind adding parenthesis? As in my $msys = (...) that is.

While you are at it, variable name is a bit misleading. mingw_on_msys would be right, but it's somewhat long...

I can go for $mom if you wish ;-)

dot-asm · 2017-09-02T17:37:40Z

Red cross from travis is from doc-nits.

levitte · 2017-09-03T08:21:41Z

Merged. 79120f4

Some URIs get "mistreated" (converted) by the MSYS run-time. Unfortunately, avoiding this conversion doesn't help either. http://www.mingw.org/wiki/Posix_path_conversion Fixes #4314 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #4322)

levitte added the branch: master Merge to master branch label Sep 1, 2017

levitte force-pushed the fix-test_store-mingw branch from c41f934 to f35ff72 Compare September 1, 2017 21:06

levitte requested a review from mattcaswell September 1, 2017 21:07

mattcaswell reviewed Sep 1, 2017

View reviewed changes

levitte force-pushed the fix-test_store-mingw branch from f35ff72 to 0cde7bd Compare September 2, 2017 05:25

OSSL_STORE: Avoid testing with URIs on the mingw command line

8305aba

Some URIs get "mistreated" (converted) by the MSYS run-time. Unfortunately, avoiding this conversion doesn't help either. http://www.mingw.org/wiki/Posix_path_conversion Fixes openssl#4314

levitte force-pushed the fix-test_store-mingw branch from 0cde7bd to 8305aba Compare September 2, 2017 08:10

dot-asm reviewed Sep 2, 2017

View reviewed changes

[to be squashed] better comment

1fabb12

dot-asm reviewed Sep 2, 2017

View reviewed changes

[to be squashed] better to check $^O

3752f44

dot-asm reviewed Sep 2, 2017

View reviewed changes

[to be squashed] update comment in the issue

f96629f

dot-asm reviewed Sep 2, 2017

View reviewed changes

levitte added 2 commits September 2, 2017 18:49

[to be squashed] better readability

31d7b3d

[to be squashed] 'built in' -> 'built-in'

05de600

dot-asm approved these changes Sep 2, 2017

View reviewed changes

dot-asm added the approval: done This pull request has the required number of approvals label Sep 2, 2017

mattcaswell approved these changes Sep 2, 2017

View reviewed changes

levitte closed this Sep 3, 2017

levitte mentioned this pull request Nov 15, 2017

pem tests on MSYS2 spurious failure #4740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSSL_STORE: Avoid testing with URIs on the command line #4322

OSSL_STORE: Avoid testing with URIs on the command line #4322

levitte commented Sep 1, 2017

mattcaswell Sep 1, 2017

levitte Sep 2, 2017

dot-asm commented Sep 1, 2017

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

dot-asm commented Sep 2, 2017

dot-asm Sep 2, 2017

levitte Sep 2, 2017

dot-asm Sep 2, 2017

levitte Sep 2, 2017

dot-asm Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

dot-asm Sep 2, 2017

dot-asm commented Sep 2, 2017

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017 •

edited

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017 •

edited

levitte commented Sep 2, 2017

dot-asm Sep 2, 2017

levitte Sep 2, 2017

dot-asm Sep 2, 2017

dot-asm Sep 2, 2017

levitte Sep 2, 2017

dot-asm commented Sep 2, 2017

levitte commented Sep 3, 2017

OSSL_STORE: Avoid testing with URIs on the command line #4322

OSSL_STORE: Avoid testing with URIs on the command line #4322

Conversation

levitte commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Sep 1, 2017

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

dot-asm commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017

Choose a reason for hiding this comment

dot-asm commented Sep 2, 2017

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017 • edited

levitte commented Sep 2, 2017

levitte commented Sep 2, 2017

dot-asm commented Sep 2, 2017 • edited

levitte commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Sep 2, 2017

levitte commented Sep 3, 2017

dot-asm commented Sep 2, 2017 •

edited

dot-asm commented Sep 2, 2017 •

edited