We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4692 - Command line parsing
Summary: Command line parsing
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: main (show other bugs)
Version: 2.0.10
Hardware: x86 Windows 10
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.12
Depends on:
Blocks:
 
Reported: 2019-06-23 11:02 UTC by Galadrim
Modified: 2019-09-20 20:48 UTC (History)
1 user (show)

See Also:

Attachments
First pass using Windows command line parsing (6.64 KB, patch)
2019-07-03 10:05 UTC, Sam Lantinga 		Details | Diff
Working version of SDL_windows_main.c (2.20 KB, text/plain)
2019-07-05 23:20 UTC, Galadrim 		Details
cleaned-up patch verison of Galadrim's SDL_windows_main.c (5.01 KB, patch)
2019-07-31 10:36 UTC, Ozkan Sezer 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Galadrim 2019-06-23 11:02:33 UTC 
As I have seen, SDL implements its own command line parser for Windows in SDL_windows_main.c. Unfortunately, it doesn't seem to allow command line arguments with trailing backslashes if quoting is required.

Usually, when you write an application that gets command line arguments passed as argc and argv, the parsing is done by parse_cmdline. The Windows API also provides the function CommandLineToArgvW, so an application can parse itself if only the command line string is provided. Both functions behave almost identically according to their documentation. If the argument "\\" (including the quotes) is passed, they both turn it into a single backslash.

The SDL command line parser on the other hand doesn't recognize the second quote character as the closing character in this example and therefore includes it in the parsed argument. The parser does not count the number of backslashes preceding a quote. It always treats a quote as escaped if a backslash is in front of it. Therefore, it should be impossible to quote and escape an argument correctly, if it has a trailing backslash and contains characters that require quoting.

Of course, each application is allowed to implement its own parsing rules, so SDL is free to do so. But the problem I see is that there are arguments, that are impossible to be passed to the parser correctly, as I described above. Is there a reason, why SDL does not simply use CommandLineToArgvW instead of implementing its own parser?

Here are some links that show that correct argument parsing, as it is usually done in Windows, is quite complicated:

https://docs.microsoft.com/en-us/windows/desktop/api/shellapi/nf-shellapi-commandlinetoargvw

http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line-into-individual-arguments/
Comment 1 Sam Lantinga 2019-07-03 10:05:47 UTC 
Created attachment 3857 [details]
First pass using Windows command line parsing

Does this patch fix the bug?
Comment 2 Galadrim 2019-07-05 21:11:54 UTC 
It does fix the bug, thanks a lot!

However I needed to make two small modifications to make it compile with Visual Studio:

1. The function CommandLineToArgvW requires shellapi.h to be included which is not yet the case.

2. In line 54, you pass a single parameter to SDL_calloc while it actually takes two parameters. So I think it should be

sizeof(*argv)

as first parameter and

argc + 1

as second one.
Comment 3 Sam Lantinga 2019-07-05 22:17:56 UTC 
Great, can you attach the working file, so I can commit it?

Thanks!
Comment 4 Galadrim 2019-07-05 23:20:39 UTC 
Created attachment 3866 [details]
Working version of SDL_windows_main.c

I attached the SDL_windows_main.c with the modifications mentioned above. I also moved the call to LocalFree directly behind the conversion from argvw to argv.
Comment 5 Ryan C. Gordon 2019-07-30 17:49:37 UTC 
(Sorry if you get several emails like this, we're marking a bunch of bugs.)

We're hoping to ship SDL 2.0.11 on a much shorter timeframe than we have historically done releases, so I'm starting to tag bugs we hope to have closed in this release cycle.

Note that this tag means we just intend to scrutinize this bug for the 2.0.11 release: we may fix it, reject it, or even push it back to a later release for now, but this helps give us both a goal and a wishlist for the next release.

If this bug has been quiet for a few months and you have new information (such as, "this is definitely still broken" or "this got fixed at some point"), please feel free to retest and/or add more notes to the bug.

--ryan.
Comment 6 Ozkan Sezer 2019-07-31 10:36:10 UTC 
Created attachment 3911 [details]
cleaned-up patch verison of Galadrim's SDL_windows_main.c

Has this got enough testing?  Attaching a cleaned-up patch
version after comment #4 (compile-tested only).
Comment 7 Sam Lantinga 2019-07-31 16:12:18 UTC 
Looks good, thanks!

I went ahead and freed argv and committed it:
https://hg.libsdl.org/SDL/rev/a1917148d38a
Comment 8 Ryan C. Gordon 2019-09-20 20:47:35 UTC 
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.
Comment 9 Ryan C. Gordon 2019-09-20 20:48:43 UTC 
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.