1

I have a program where I need to execute an .exe to convert images to DDS before I can use them in my program. To do this, I have a function where I can pass a command to system() like so:

unsigned int __stdcall Scene::ExecuteCommand(void* command)
{
    return system(static_cast<char*>(command));
}

I was thinking the other day that perhaps this is unsafe/dangerous but I'm not 100% sure if it is or not (and if it is, how it is unsafe/dangerous). The function works as intended but is this bad practice? Should I be concerned?

My question is not a duplicate because I am asking about the specifics and the impact of using system() inside of a function. My function allows for any command line argument to be passed to system in a multi-threaded fashion.

Katianie
  • 589
  • 1
  • 9
  • 38
  • 4
    Generally, yes, using `system` is considered bad practice. I'm not sure what you mean by "public function". This is a class member function that you've declared public? Are you distributing this code as a library? What type of "safety" are you worried about? – Cody Gray - on strike Jul 01 '16 at 17:37
  • See also: http://stackoverflow.com/questions/19913446/why-should-the-system-function-be-avoided-in-c-and-c – Cody Gray - on strike Jul 01 '16 at 17:38
  • 1
    Also remember that while your external program is running, the `system` function does not return so if it takes some time your application might seem (and actually be) unresponsive. – Some programmer dude Jul 01 '16 at 17:39
  • Why are you passing a `void*` as parameter there? – πάντα ῥεῖ Jul 01 '16 at 17:40
  • Correct about it not returning, in this secnario that is not a big issue. I'm passing void because when using windows multithreadding, I need to have the parameters as such. @CodyGray This is not for a library but the function can be accessed from outside that class (thats what I meen by public). The only way to convert an image to DDS on the fly is to use this application. This lead me to have to run the application using system() – Katianie Jul 01 '16 at 17:40
  • Windows multithreading does not enforce the use of void pointers. Even if you're using a function that forces you to pass data as a pointer-sized argument (LPARAM or whatever), you could dereference the pointer back to the actual type before making this call. And public/private access modifiers, assuming you're not distributing the code as a library, don't have nearly the effect that you seem to think they do. They just enforce a logical separation of concerns. There is nothing "unsafe" about a public class member, unless you can't trust yourself and the other people on your team. – Cody Gray - on strike Jul 01 '16 at 17:45
  • Its not exactly a duplicate because I am asking about using it inside a function that uses it....But I agree It is similar. Its not that I don't trust myself, its that I wonder if its possible to "hack" my application and get access to that function which in turn could make it dangerous? I don't know, just a thought. – Katianie Jul 01 '16 at 17:49
  • @CodyGray When I change the parameter to char*, I get the following error: 'uintptr_t _beginthreadex(void *,unsigned int,_beginthreadex_proc_type,void *,unsigned int,unsigned int *)': cannot convert argument 3 from 'unsigned int (__stdcall *)(char *)' to '_beginthreadex_proc_type' – Katianie Jul 01 '16 at 17:56
  • @Katianie Your question is a duplicate, and security concerns are covered [here](http://stackoverflow.com/a/28825220/1413395). It doesn't matter much if the parameter was passed into a function, or inline from sequential code. – πάντα ῥεῖ Jul 01 '16 at 17:59
  • I guess @πάνταῥεῖ decided on his own to close the question even though it is not a duplicate. – Katianie Jul 01 '16 at 17:59
  • @Katianie Point of duplicates are the existing answers, not the exact wording of the question. – πάντα ῥεῖ Jul 01 '16 at 18:01
  • @πάνταῥεῖ I strongly disagree, not all people learn/understand things the same way they are said, but it doesn't matter because you have more internet points then me. – Katianie Jul 01 '16 at 18:06
  • @Katianie _"not all people learn/understand things the same way they are said"_ Being able to extrapolate on situations, and get solutions from similar ones is an essential skill in our business. You should absolutely learn that. Regarding your disagreement, please follow the recommendations [from here](http://meta.stackoverflow.com/questions/253521/what-can-i-do-if-i-believe-that-my-question-was-wrongly-marked-as-a-duplicate) please. – πάντα ῥεῖ Jul 01 '16 at 18:10

1 Answers1

2

This is a relatively unsafe practice : this function risks to become a source of weakness if it is called with unchecked parameters.

Example: 

string my_clean_command{"convertdss.exe -n "};    // we have best intentions
string my_user_param;                             // but we don't know the user
cout<<"Enter file to convert: ";
getline (my_user_param);   // user enters: "image.jpg | rm *.*"
string my_corrupted_command = my_clean_command + my_user_param;   // ouch!!
my_scene.ExecuteCommand(my_corrupted_command.c_str());            // ouch!!!

As the command is executed by the local command processor, it will be source of non-portability (or surprises if a user has configured his session to use an unusual command processor).

Finally, you'll launch a distinct process, creating some system overhead that is not absolutely necessary.

Christophe
  • 68,716
  • 7
  • 72
  • 138
  • 1
    But how is it unsafe exactly? The arguments I'm passing in are independent and not specific to my computer. What is the correct alternative to run an application and pass it a command line argument if system() is considered unsafe practice? – Katianie Jul 01 '16 at 17:52
  • 1
    @Katianie I've edited to show you how this can happen. Of course this is a very simple example ans stupid attack. But you can't imagine how many websites got comprimized though command or sql injection in user input !! – Christophe Jul 01 '16 at 18:09
  • 1
    @Katianie [Seriously?](https://xkcd.com/327/) :-) ... – πάντα ῥεῖ Jul 01 '16 at 18:34
  • @πάνταῥεῖ :-D Excellent ! – Christophe Jul 01 '16 at 18:42
  • 1
    @Katianie If you have to call an external exe, a safer alternative would be to have an `Scene::ExecuteConversion()` that would take the arguments you intend to use to construct your command, and check them before issuing the call to `system()`. That would be far better from encapsulation point of view, and it would allow for a systematic parameter control. And it would isolate OS dependent code (e.g. using - or / for command options). Finally, and although it's not part of the standard, I'd opt for POSIX `execvp()` (`_execvp` for windows): it doesn't interpret the passed arguments. – Christophe Jul 01 '16 at 19:04