cimg::system() should use CreateProcessA

[multiplemonomials]

I was running into some issues with using an ImageMagick path containing spaces under Windows, and tracked it down to this problem plus another issue. cimg::system() has the following code:

const BOOL res = CreateProcess((LPCTSTR)module_name,(LPTSTR)command,0,0,FALSE_WIN,0,0,0,&si,&pi);

module_name and command are ASCII C strings. However, when building in Unicode mode, we cast the pointers to TCHARs and pass them to CreateProcess. This causes CreateProcess to parse the ASCII strings' data as if they were WCHAR-strings -- so it reads garbage data for the path and command, and isn't able to execute the program!
The fix for this is to explicitly call CreateProcessA, which accepts ASCII strings, and also to make a few other type changes in the function to account for this.

[dtschump]

The fix for this is to explicitly call CreateProcessA, which accepts ASCII strings, and also to make a few other type changes in the function to account for this.

I don't have a Windows machine to test right here. Any chance you can create a PR for this ? Thanks !

[multiplemonomials]

Sure, I can do that.

It looks like the current cimg::system() function is kind of working by accident -- the CreateProcess call always fails under Windows, but there's a "backup" std::system() call that is used at the end. This normally lets things keep working, it's just that it breaks with program paths with spaces, which is how this whole thing got started for me. I'm going to go ahead and delete that since all process creating needs should now be handled by the CreateProcessA call.

Also, is it OK to include the <system_error> header? Or is there another way that you are using to get the string for an error code on Windows?

[dtschump]

Great news!

Also, is it OK to include the header? Or is there another way that you are using to get the string for an error code on Windows?

From what I see, this header has been introduced with C++11, which is a bit annoying, because I'm trying to keep C++-98 as much as possible for CImg (which is AFAIK still used with older compilers). If we can avoid breaking C++-98 compatibility for managing error code purposes, it could be better. Anyway, any help is appreciated, so feel free to use it if necessary, and I'll probably add macro conditional afterwards, as #if cimg_use_cpp11==1... #endif to ignore it one way or another afterwards.

Thanks!

[dtschump]

Also, if possible, please write the PR against the develop branch of CImg. That would be easier for me to work on it.

[multiplemonomials]

OK PR is pushed.

One other thing: I feel like it would really be worth cleaning up the code for the imagemagick_path, gzip_path, etc. family of functions. There's a lot of copy-pasted code between all of them. Maybe you could combine them into a single get_exe_path function that takes an enum for which one to get, stores the data in a map, and uses the specific paths and names for the enum values you pass? Just my suggestion though.

[dtschump]

Thanks for the PR, that looks really good. Concerning the error message : I think cimg::system() is always called in a context where, when it fails, there is already an error message and a CImgException thrown by the calling function. I suggest then we use cimg::warn() to display the error message specific to the call to CreateProcessA(). On Linux, I don't think there will be any error message displaying with cimg::system() (although it could be a good idea to add the same kind of warning, I'll see if it's possible later).

For the various *_path() function : you are absolutely right. I'll probably have a look at this tomorrow.

Thanks again for the nice contribution.