Re: [PATCH] programs/explorer: Fix explorer's command line parsing (try 2)
On 01/08/2012 08:45 PM, Jay Yang wrote:
Fixed some small formatting issues from last time.
Your patch strips spaces all over the place. For example: + LPWSTR *argv; + if(commandline[0]=='\0') Need an empty line after variable declarations. No space after "if". No space around equal operator. + for(i=0;i<argc;i++) No spaces after semi-colon. + copy_path_string(parameters->root,curr); No space after coma. Some other issues - you will parse over parameters given to the program. Ex: explorer /desktop=foo,800x600 program.exe /n /e Parsing inside parameters is incorrect: + LPWSTR end=strchrW(curr,','); + LPWSTR next; + if(end==NULL) + next=(end=strchrW(curr,'\0')); + else + next=end+1; ... + curr=next; You end up parsing something like this as a valid param: /n,/e,/select,c:\\ Vitaliy.
On 01/09/2012 09:46 AM, Vitaliy Margolen wrote:
On 01/08/2012 08:45 PM, Jay Yang wrote:
Fixed some small formatting issues from last time.
Your patch strips spaces all over the place. For example: + LPWSTR *argv; + if(commandline[0]=='\0') Need an empty line after variable declarations. No space after "if". No space around equal operator.
+ for(i=0;i<argc;i++) No spaces after semi-colon.
+ copy_path_string(parameters->root,curr); No space after coma.
I'll fix these
Some other issues - you will parse over parameters given to the program. Ex: explorer /desktop=foo,800x600 program.exe /n /e
the "/desktop" command causes a the parsing to go back to the original command line and reparse it. I can add a comment for this. So in this particular case, manage_desktop is called with "=foo,800x600 program.exe /n /e", which should be correct
Parsing inside parameters is incorrect: + LPWSTR end=strchrW(curr,','); + LPWSTR next; + if(end==NULL) + next=(end=strchrW(curr,'\0')); + else + next=end+1; ... + curr=next;
You end up parsing something like this as a valid param: /n,/e,/select,c:\\ Windows accepts this. If one types explorer /n,/e,/select,c:\ into the windows command line, it opens My Computer with the c:\ drive selected. Incidentally this is not quite what wine does because copy_path_root only tries to strip the last part of the path and the wine explorer currently doesn't actually select anything, even with the select parameter. Should I fix this in this patch or another patch?
On 01/09/2012 08:34 AM, Jay Yang wrote:
On 01/09/2012 09:46 AM, Vitaliy Margolen wrote:
On 01/08/2012 08:45 PM, Jay Yang wrote:
Some other issues - you will parse over parameters given to the program. Ex: explorer /desktop=foo,800x600 program.exe /n /e
the "/desktop" command causes a the parsing to go back to the original command line and reparse it. I can add a comment for this. So in this particular case, manage_desktop is called with "=foo,800x600 program.exe /n /e", which should be correct That's the wrong way to do it. The "/desktop" string can be anywhere in the command line. And you not accounting for any sort of quotes.
You end up parsing something like this as a valid param: /n,/e,/select,c:\\ Windows accepts this. If one types explorer /n,/e,/select,c:\ into the windows command line, it opens My Computer with the c:\ drive selected. Incidentally this is not quite what wine does because Interesting, Windows has it worse then I thought (what a surprise). Yeah that appears to work on native.
copy_path_root only tries to strip the last part of the path and the wine explorer currently doesn't actually select anything, even with the select parameter. Should I fix this in this patch or another patch? Yes separate patch please.
Vitaliy
participants (2)
-
Jay Yang -
Vitaliy Margolen