Bug 16127 - system2 arguments are not wrapped with shQuote
Summary: system2 arguments are not wrapped with shQuote
Status: NEW
Alias: None
Product: R
Classification: Unclassified
Component: Windows GUI / Window specific (show other bugs)
Version: R 3.1.2
Hardware: x86_64/x64/amd64 (64-bit) Windows 64-bit
: P5 enhancement
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2014-12-30 20:46 UTC by Neil Schneider
Modified: 2017-01-17 18:18 UTC (History)
1 user (show)

See Also:


Attachments
system2 function with arguments wrapped with shQuote (5.25 KB, text/plain)
2014-12-30 20:46 UTC, Neil Schneider
Details
File to be passed arguments. (56 bytes, text/plain)
2014-12-30 20:47 UTC, Neil Schneider
Details
system.R with system2 that maintains backward compatibility (5.41 KB, text/plain)
2015-02-13 16:12 UTC, Neil Schneider
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Neil Schneider 2014-12-30 20:46:47 UTC
Created attachment 1702 [details]
system2 function with arguments wrapped with shQuote

The system2 function wraps the command with shQuote, but does not wrap the arguments. This can cause unintended results if the arguments contains file paths that include spaces.

This can be reproduced using the following code and the attached example.R script. (example.R should be in a file path with a space.)

#Fatal error: cannot open file 'C:/Temp': Permission denied
system2("RScript.exe", "C:/Temp files/example.R")

#Prints "Hello World!" and character(0) since there are no additional arguments.
system2("RScript.exe", shQuote("C:/Temp files/example.R"))

#Sending additional arguments to example.R will split the second argument at the spaces.
system2("RScript.exe", args=c(shQuote("C:/Temp files/example.R"), "arg1", "arg2 with spaces", "arg3"))

I have also attached an updated system.R file which will individually wrap the arguments using shQuote and sapply. Line 73-74:
    command <- paste(c(shQuote(command), env,
                       sapply(args, shQuote, USE.NAMES=FALSE)), collapse = " ")

The documentation on system2 states that args is "a character vector of arguments to command". The current code does not differentiate between args="arg1 arg2 arg3" and args=c("arg1", "arg2", "arg3"). This proposed enhancement would better match the documentation, but has the potential to cause problems with people who have used system2 in the non-vector space delimited argument format.
Comment 1 Neil Schneider 2014-12-30 20:47:35 UTC
Created attachment 1703 [details]
File to be passed arguments.
Comment 2 Neil Schneider 2015-02-13 16:12:37 UTC
Created attachment 1744 [details]
system.R with system2 that maintains backward compatibility

This is an updated version of the system.R. This version maintains backward compatibility of system2, while adding the requested feature of shQuoting the arguments as well as the command.
Comment 3 Neil Schneider 2015-02-13 16:22:39 UTC
Test code would now look like this:

#Does not run
> system2("RScript.exe", args=c("C:/Temp files/example.R", "arg1", "arg2 with spaces", "arg3"))

Fatal error: cannot open file 'C:/Temp': Permission denied

Warning message:
running command '"RScript.exe" C:/Temp files/example.R arg1 arg2 with spaces arg3' had status 2 

#Runs with arguments split on spaces
> system2("RScript.exe", args=c(shQuote("C:/Temp files/example.R"), "arg1", "arg2 with spaces", "arg3"))

[1] "hello world!"
[1] "arg1"   "arg2"   "with"   "spaces" "arg3" 

#Runs with arguments shQuoted
> system2("RScript.exe", args=c("C:/Temp files/example.R", "arg1", "arg2 with spaces", "arg3"), quote.args = TRUE)

[1] "hello world!"
[1] "arg1"             "arg2 with spaces" "arg3"
Comment 4 Frederick Eaton 2017-01-17 18:18:15 UTC
I noticed this "bug" as well. It's confusing because

- The system2 help page prominently mentions quoting as a distinguishing feature: "Unlike ‘system’, ‘command’ is always quoted by ‘shQuote’, so it must be a single command without arguments."

- The arguments for system2 are called "args" and referred to as a "character vector". What's the point of letting the user specify separate arguments if they're just going to be pasted together?

Certainly the documentation should be fixed, but I think the functionality too.

I support Neil's patch, but it's unfortunate that users will now have to put 'quote.args=T' after every system2 invocation. It's almost preferable to deprecate 'system2' and introduce a 'system3' with good defaults. Or make quote.args=T the default. It's not just a UI issue but a security issue as well.

Note that "src/library/base/R/unix/system.unix.R" also needs to be updated, as well as the documentation in "src/library/base/man/system2.Rd".

Note also that 'shQuote' takes character vector arguments, so I don't see the need for sapply in Neil's patch.

Thank you Neil.