Remove dependency on shell32.dll by notriddle · Pull Request #56568 · rust-lang/rust

notriddle · 2018-12-06T18:32:36Z

Closes #56510 if it works on MinGW (I've only tested it on MSVC).

rust-highfive · 2018-12-06T18:32:46Z

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2018-12-06T18:35:44Z

It'd be good to also run this on Windows a couple times to attempt to ascertain performance impact on our tests, though we could also just land it and see what AppVeyor looks like over the next dozen builds.

r? @alexcrichton

notriddle · 2018-12-06T18:42:08Z

micha@DESKTOP-IIQA1VP MINGW64 ~/IdeaProjects/rust (master)
$ ldd build/x86_64-pc-windows-msvc/stage1-std/x86_64-pc-windows-msvc/release/std.dll
        ntdll.dll => /c/WINDOWS/SYSTEM32/ntdll.dll (0x7ffaa7f10000)
        KERNEL32.DLL => /c/WINDOWS/System32/KERNEL32.DLL (0x7ffaa6cc0000)
        KERNELBASE.dll => /c/WINDOWS/System32/KERNELBASE.dll (0x7ffaa44d0000)
        ADVAPI32.dll => /c/WINDOWS/System32/ADVAPI32.dll (0x7ffaa7510000)
        msvcrt.dll => /c/WINDOWS/System32/msvcrt.dll (0x7ffaa56b0000)
        sechost.dll => /c/WINDOWS/System32/sechost.dll (0x7ffaa7620000)
        RPCRT4.dll => /c/WINDOWS/System32/RPCRT4.dll (0x7ffaa5750000)
        WS2_32.dll => /c/WINDOWS/System32/WS2_32.dll (0x7ffaa6ef0000)
        USERENV.dll => /c/WINDOWS/SYSTEM32/USERENV.dll (0x7ffaa4150000)
        ucrtbase.dll => /c/WINDOWS/System32/ucrtbase.dll (0x7ffaa52a0000)
        profapi.dll => /c/WINDOWS/System32/profapi.dll (0x7ffaa4240000)
        CRYPTBASE.DLL => /c/WINDOWS/SYSTEM32/CRYPTBASE.DLL (0x7ffaa3b50000)
        bcryptPrimitives.dll => /c/WINDOWS/System32/bcryptPrimitives.dll (0x7ffaa4970000)

micha@DESKTOP-IIQA1VP MINGW64 ~/IdeaProjects/rust (master)
$ ldd ~/.rustup/toolchains/stable-x86_64-pc-windows-msvc/lib/rustlib/x86_64-pc-windows-msvc/lib/std-66ce4ddf5a45ca83.dll
        ntdll.dll => /c/WINDOWS/SYSTEM32/ntdll.dll (0x7ffaa7f10000)
        KERNEL32.DLL => /c/WINDOWS/System32/KERNEL32.DLL (0x7ffaa6cc0000)
        KERNELBASE.dll => /c/WINDOWS/System32/KERNELBASE.dll (0x7ffaa44d0000)
        ADVAPI32.dll => /c/WINDOWS/System32/ADVAPI32.dll (0x7ffaa7510000)
        msvcrt.dll => /c/WINDOWS/System32/msvcrt.dll (0x7ffaa56b0000)
        sechost.dll => /c/WINDOWS/System32/sechost.dll (0x7ffaa7620000)
        RPCRT4.dll => /c/WINDOWS/System32/RPCRT4.dll (0x7ffaa5750000)
        WS2_32.dll => /c/WINDOWS/System32/WS2_32.dll (0x7ffaa6ef0000)
        SHELL32.dll => /c/WINDOWS/System32/SHELL32.dll (0x7ffaa5880000)
        cfgmgr32.dll => /c/WINDOWS/System32/cfgmgr32.dll (0x7ffaa47b0000)
        ucrtbase.dll => /c/WINDOWS/System32/ucrtbase.dll (0x7ffaa52a0000)
        USERENV.dll => /c/WINDOWS/SYSTEM32/USERENV.dll (0x7ffaa4150000)
        shcore.dll => /c/WINDOWS/System32/shcore.dll (0x7ffaa5530000)
        combase.dll => /c/WINDOWS/System32/combase.dll (0x7ffaa7b90000)
        bcryptPrimitives.dll => /c/WINDOWS/System32/bcryptPrimitives.dll (0x7ffaa4970000)
        windows.storage.dll => /c/WINDOWS/System32/windows.storage.dll (0x7ffaa49f0000)
        shlwapi.dll => /c/WINDOWS/System32/shlwapi.dll (0x7ffaa7aa0000)
        GDI32.dll => /c/WINDOWS/System32/GDI32.dll (0x7ffaa6e40000)
        gdi32full.dll => /c/WINDOWS/System32/gdi32full.dll (0x7ffaa5100000)
        msvcp_win.dll => /c/WINDOWS/System32/msvcp_win.dll (0x7ffaa48d0000)
        profapi.dll => /c/WINDOWS/System32/profapi.dll (0x7ffaa4240000)
        USER32.dll => /c/WINDOWS/System32/USER32.dll (0x7ffaa53a0000)
        win32u.dll => /c/WINDOWS/System32/win32u.dll (0x7ffaa4800000)
        kernel.appcore.dll => /c/WINDOWS/System32/kernel.appcore.dll (0x7ffaa4270000)
        powrprof.dll => /c/WINDOWS/System32/powrprof.dll (0x7ffaa4290000)
        FLTLIB.DLL => /c/WINDOWS/System32/FLTLIB.DLL (0x7ffaa4260000)
        CRYPTBASE.DLL => /c/WINDOWS/SYSTEM32/CRYPTBASE.DLL (0x7ffaa3b50000)
        IMM32.DLL => /c/WINDOWS/System32/IMM32.DLL (0x7ffaa7b00000)

alexcrichton

Thanks for this! Naturally my biggest worry here is that this may not be compatible with the previous API call, so we need to be super careful. The documentation for CommandLineToArgvW says some things like:

However, if lpCmdLine starts with any amount of whitespace, CommandLineToArgvW will consider the first argument to be an empty string. Excess whitespace at the end of lpCmdLine is ignored.

but that may not be handled here?

I think this otherwise looks pretty equivalent though. Could this perhaps be tested locally more rigorously? Could the output of CommandLineToArgvW be compared to this implementation for a number of procedurally generated inputs?

pitdicker · 2018-12-07T20:15:12Z

Does it help to compare the implementation in Wine and ReactOS? https://doxygen.reactos.org/da/da5/shell32__main_8c_source.html

nagisa · 2018-12-07T22:27:49Z

Seems like a great thing to throw at a fuzzer of some sort for a day or two, before it is merged.

Among other things this would not only verify the correctness of the code, but also give considerable degree of confidence about the equivalence of the function(s).

notriddle · 2018-12-08T06:01:37Z

Okay, here's a test harness that I put together to compare the behavior of the two (it is exhaustive, rather than random, so it should be pretty thorough).

https://gist.github.com/notriddle/dde431930c392e428055b2dc22e638f5

https://paste.gg/p/anonymous/47d6ed5f5bd549168b1c69c799825223

And, yeah, I've found several inconsistencies. The ReactOS code was also helpful for figuring out what was wrong; thanks @pitdicker !

notriddle · 2018-12-09T16:27:17Z

ucs_2=[97, 32, 34, 1, 34, 34, 0]
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `["a", "\u{1}\""]`,
 right: `["a", "\u{1}"]`', .\test_parser.rs:215:25

Well, there's another thing I need to figure out.

notriddle · 2018-12-10T16:41:58Z

The exhaustive test now finishes without producing any errors!

alexcrichton · 2018-12-10T19:02:58Z

It looks like some submodule updates may have snuck in here by accident? Otherwise this seems like it's likely good to go perhaps?

notriddle · 2018-12-10T19:13:13Z

@alexcrichton Fixed.

notriddle · 2018-12-11T19:43:44Z

That should fix both of your nits, @alexcrichton

alexcrichton · 2018-12-11T21:57:19Z

@bors: r+

💯

bors · 2018-12-11T21:57:20Z

📌 Commit 83fe6e4 has been approved by alexcrichton

bors · 2018-12-11T23:38:16Z

⌛ Testing commit 83fe6e4 with merge a8aa87621b2bb1f806793c93b5eea25649b7e651...

bors · 2018-12-11T23:40:05Z

💔 Test failed - status-appveyor

alexcrichton · 2018-12-11T23:46:56Z

@bors: retry

CI automation hiccup

kennytm · 2018-12-14T13:44:08Z

@bors p=1 retry

bors · 2018-12-14T13:44:22Z

⌛ Testing commit 83fe6e4 with merge 7d03617...

Remove dependency on shell32.dll Closes #56510 if it works on MinGW (I've only tested it on MSVC).

bors · 2018-12-14T16:15:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 7d03617 to master...

arsing · 2019-02-22T02:21:38Z

Just wanted to say this is also useful for getting Rust binaries running inside nanoserver containers, since those don't have shell32.dll. Thanks!

(Might be worth advertising this in the 1.33 release post :) )

lambda-fairy · 2019-03-01T06:44:32Z

+            end += 1;
+        }
+        slice::from_raw_parts(lp_cmd_line, end as usize)
+    };


Would it be better to split this pointer manipulation into a separate function?

It looks like there's no actual unsafe code below this point, but you wouldn't know that without digging through the whole implementation.

* Respect starting directory for dialogs on Windows Before this change, FileDialogOptions::starting_directory had not been used on Windows and therefore had no influence on the starting directory of a FileDialog window. This has now been fixed. The use of `SHCreateItemFromParsingName` makes it mandatory to explicitly link against shell32.lib because this is not done automatically since Rust 1.33.0. See rust-lang/rust#56568 for details. * Fix: Use winapi feature instead of build.rs As it turns out, the winapi crate already exposes shell32.lib via a cargo feature, so there is no need to link it manually.

rust-highfive assigned dtolnay Dec 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2018

rust-highfive assigned alexcrichton and unassigned dtolnay Dec 6, 2018

This comment has been minimized.

Sign in to view

pitdicker reviewed Dec 7, 2018

View reviewed changes

Comment thread src/libstd/sys/windows/args.rs Outdated

alexcrichton reviewed Dec 7, 2018

View reviewed changes

Comment thread src/libstd/sys/windows/args.rs Outdated

Comment thread src/libstd/sys/windows/args.rs Outdated

Comment thread src/libstd/sys/windows/args.rs Outdated

Comment thread src/libstd/sys/windows/args.rs Outdated

Comment thread src/libstd/sys/windows/args.rs Outdated

This comment has been minimized.

Sign in to view

retep998 reviewed Dec 8, 2018

View reviewed changes

Comment thread src/libstd/sys/windows/args.rs Outdated

alexcrichton reviewed Dec 8, 2018

View reviewed changes

Comment thread src/libstd/sys/windows/args.rs Outdated

This comment has been minimized.

Sign in to view

notriddle force-pushed the master branch 2 times, most recently from 2de79c1 to f6e7c37 Compare December 9, 2018 22:41

This comment has been minimized.

Sign in to view

notriddle force-pushed the master branch from f6e7c37 to 6846825 Compare December 10, 2018 16:43

Remove dependency on shell32.dll rust-lang#56510

81de5d9

notriddle force-pushed the master branch from 6846825 to 81de5d9 Compare December 10, 2018 19:13

alexcrichton reviewed Dec 10, 2018

View reviewed changes

Comment thread src/libstd/sys/windows/args.rs Outdated

Fix iterator nits

55420f0

Use iterators instead of raw offsets in Windows argument parser

83fe6e4

notriddle force-pushed the master branch from 53e09b6 to 83fe6e4 Compare December 11, 2018 20:06

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 11, 2018

This comment has been minimized.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2018

bors added a commit that referenced this pull request Dec 14, 2018

Auto merge of #56568 - notriddle:master, r=alexcrichton

7d03617

Remove dependency on shell32.dll Closes #56510 if it works on MinGW (I've only tested it on MSVC).

bors merged commit 83fe6e4 into rust-lang:master Dec 14, 2018

nikic mentioned this pull request Dec 15, 2018

MSVC link errors with __imp_ symbols #56839

Closed

briansmith mentioned this pull request Feb 28, 2019

Investigate impact of removing shell32 dependency of libstd on Windows #56510

Closed

lambda-fairy reviewed Mar 1, 2019

View reviewed changes

0x08088405 mentioned this pull request Mar 6, 2019

[Windows] bundle + static-link - shell32 not linking, possible relation to rust pull #56568 (1.33+) Rust-SDL2/rust-sdl2#864

Open

udoprog mentioned this pull request Mar 9, 2019

Windows linker error on nightly Rust (unresolved external symbol __imp_ExtractIconW referenced in function get_app_icon) hrkfdn/ncspot#24

Closed

MaximilianKoestler mentioned this pull request Dec 11, 2020

Respect starting directory for dialogs on Windows linebender/druid#1452

Merged

Conversation

notriddle commented Dec 6, 2018

Uh oh!

rust-highfive commented Dec 6, 2018

Uh oh!

Mark-Simulacrum commented Dec 6, 2018

Uh oh!

notriddle commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitdicker commented Dec 7, 2018

Uh oh!

nagisa commented Dec 7, 2018

Uh oh!

This comment has been minimized.

notriddle commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

notriddle commented Dec 9, 2018

Uh oh!

This comment has been minimized.

notriddle commented Dec 10, 2018

The exhaustive test now finishes without producing any errors!

Uh oh!

alexcrichton commented Dec 10, 2018

Uh oh!

notriddle commented Dec 10, 2018

Uh oh!

Uh oh!

notriddle commented Dec 11, 2018

Uh oh!

alexcrichton commented Dec 11, 2018

Uh oh!

bors commented Dec 11, 2018

Uh oh!

bors commented Dec 11, 2018

Uh oh!

bors commented Dec 11, 2018

Uh oh!

This comment has been minimized.

alexcrichton commented Dec 11, 2018

Uh oh!

kennytm commented Dec 14, 2018

Uh oh!

bors commented Dec 14, 2018

Uh oh!

bors commented Dec 14, 2018

Uh oh!

arsing commented Feb 22, 2019

Uh oh!

lambda-fairy Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

notriddle commented Dec 6, 2018 •

edited

Loading

notriddle commented Dec 8, 2018 •

edited

Loading