Re: [PATCH v2] kernel32: Use setprogname(3) to set the program name if available.
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init. Why wouldn’t setprogname(argv[1]) be adequate? ~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com> --- Try 2: * Clean up the #ifdefs a bit. (I'm not happy about the duplicated code, though.) * Don't copy the progname into a static array. There's no need; the argument vector lasts as long as the process itself. (I suppose I was being overly defensive with that.)
configure.ac | 1 + dlls/kernel32/process.c | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac index c9445e7..2ebbb10 100644 --- a/configure.ac +++ b/configure.ac @@ -2026,6 +2026,7 @@ AC_CHECK_FUNCS(\ sched_yield \ select \ setproctitle \ + setprogname \ setrlimit \ settimeofday \ sigaltstack \ diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c index 86333be..4771108 100644 --- a/dlls/kernel32/process.c +++ b/dlls/kernel32/process.c @@ -1118,9 +1118,20 @@ static void set_process_name( int argc, char *argv[] ) { #ifdef HAVE_SETPROCTITLE setproctitle("-%s", argv[1]); -#endif + /* remove argv[0] */ + memmove( argv, argv + 1, argc * sizeof(argv[0]) ); +#elif defined(HAVE_SETPROGNAME) + int i, offset; + char *end = argv[argc-1] + strlen(argv[argc-1]) + 1;
-#ifdef HAVE_PRCTL + offset = argv[1] - argv[0]; + memmove( argv[1] - offset, argv[1], end - argv[1] ); + memset( end - offset, 0, offset ); + for (i = 1; i < argc; i++) argv[i-1] = argv[i] - offset; + argv[i-1] = NULL; + + setprogname( argv[0] ); +#elif defined(HAVE_PRCTL) int i, offset; char *p, *prctl_name = argv[1]; char *end = argv[argc-1] + strlen(argv[argc-1]) + 1; @@ -1141,11 +1152,11 @@ static void set_process_name( int argc, char *argv[] ) argv[i-1] = NULL; } else -#endif /* HAVE_PRCTL */ { /* remove argv[0] */ memmove( argv, argv + 1, argc * sizeof(argv[0]) ); } +#endif /* HAVE_PRCTL */ }
-- 2.7.1
I got the CC wrong on the email. Sorry for the wine-patches noise. ~Theodore
On Apr 6, 2016, at 1:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com> --- Try 2: * Clean up the #ifdefs a bit. (I'm not happy about the duplicated code, though.) * Don't copy the progname into a static array. There's no need; the argument vector lasts as long as the process itself. (I suppose I was being overly defensive with that.)
configure.ac | 1 + dlls/kernel32/process.c | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac index c9445e7..2ebbb10 100644 --- a/configure.ac +++ b/configure.ac @@ -2026,6 +2026,7 @@ AC_CHECK_FUNCS(\ sched_yield \ select \ setproctitle \ + setprogname \ setrlimit \ settimeofday \ sigaltstack \ diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c index 86333be..4771108 100644 --- a/dlls/kernel32/process.c +++ b/dlls/kernel32/process.c @@ -1118,9 +1118,20 @@ static void set_process_name( int argc, char *argv[] ) { #ifdef HAVE_SETPROCTITLE setproctitle("-%s", argv[1]); -#endif + /* remove argv[0] */ + memmove( argv, argv + 1, argc * sizeof(argv[0]) ); +#elif defined(HAVE_SETPROGNAME) + int i, offset; + char *end = argv[argc-1] + strlen(argv[argc-1]) + 1;
-#ifdef HAVE_PRCTL + offset = argv[1] - argv[0]; + memmove( argv[1] - offset, argv[1], end - argv[1] ); + memset( end - offset, 0, offset ); + for (i = 1; i < argc; i++) argv[i-1] = argv[i] - offset; + argv[i-1] = NULL; + + setprogname( argv[0] ); +#elif defined(HAVE_PRCTL) int i, offset; char *p, *prctl_name = argv[1]; char *end = argv[argc-1] + strlen(argv[argc-1]) + 1; @@ -1141,11 +1152,11 @@ static void set_process_name( int argc, char *argv[] ) argv[i-1] = NULL; } else -#endif /* HAVE_PRCTL */ { /* remove argv[0] */ memmove( argv, argv + 1, argc * sizeof(argv[0]) ); } +#endif /* HAVE_PRCTL */ }
-- 2.7.1
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to. -Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup. ~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
Ah, I see. Yeah, that's weird. Have you tried asking Chip directly? I'm CC'ing him. -Ken
On Apr 6, 2016, at 4:53 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup.
~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
I CC’d him on the original email. It would be neat if the wine program name could show up in Activity Monitor on OSX, so I can tell one wine process apart from another. There is a way to do it (http://stackoverflow.com/questions/4217947/setting-process-name-on-mac-os-x-...) but it requires undocumented APIs. (Damn.) ~Theodore
On Apr 6, 2016, at 3:35 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Ah, I see. Yeah, that's weird. Have you tried asking Chip directly? I'm CC'ing him.
-Ken
On Apr 6, 2016, at 4:53 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup.
~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
Sorry it took me so long to respond. What this actually does is it changes the ps output so that the wine binary doesn't show up in the command line. Naturally, it relies on the argv block being one huge null-null-terminated list of strings (as most UNIX-like systems do, apparently). If you're not building your argv this way, then yes, wine will crash. Also, if wine's idea of the argv block isn't the one that the OS gave to main(), the ps output won't be affected. The code that manipulates the argv block existed in the Linux case. I surmise, then, that we'd have this same problem there. I'd kinda like to keep it, but then we'd need some way to detect that the argv we have isn't the block that was passed to main(), or isn't a null-null-terminated string list. (Or, maybe I can use _NSGetArgv() on Mac OS... but then all the other systems we support will still have this problem, and since you've written your own loader, we can't really rely on the order of arguments being "wine" "program" "...".) If you'd like me to just gut that piece of code, I'll be happy to write a patch to do that. Chip On Wed, Apr 6, 2016 at 4:03 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
I CC’d him on the original email.
It would be neat if the wine program name could show up in Activity Monitor on OSX, so I can tell one wine process apart from another. There is a way to do it ( http://stackoverflow.com/questions/4217947/setting-process-name-on-mac-os-x-...) but it requires undocumented APIs. (Damn.)
~Theodore
On Apr 6, 2016, at 3:35 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Ah, I see. Yeah, that's weird. Have you tried asking Chip directly? I'm CC'ing him.
-Ken
On Apr 6, 2016, at 4:53 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup.
~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
I have both a custom wine loader and a hacked version of Wine, so I just reverted the patch. It would be really nice to have this work for when I’m not using a custom loader. Maybe you could check if argv is in the malloc heap? Or maybe I could somehow indicate when calling wine_init that the argv is malloced? ~Theodore
On Apr 26, 2016, at 1:02 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Sorry it took me so long to respond.
What this actually does is it changes the ps output so that the wine binary doesn't show up in the command line. Naturally, it relies on the argv block being one huge null-null-terminated list of strings (as most UNIX-like systems do, apparently). If you're not building your argv this way, then yes, wine will crash. Also, if wine's idea of the argv block isn't the one that the OS gave to main(), the ps output won't be affected.
The code that manipulates the argv block existed in the Linux case. I surmise, then, that we'd have this same problem there.
I'd kinda like to keep it, but then we'd need some way to detect that the argv we have isn't the block that was passed to main(), or isn't a null-null-terminated string list. (Or, maybe I can use _NSGetArgv() on Mac OS... but then all the other systems we support will still have this problem, and since you've written your own loader, we can't really rely on the order of arguments being "wine" "program" "...".) If you'd like me to just gut that piece of code, I'll be happy to write a patch to do that.
Chip
On Wed, Apr 6, 2016 at 4:03 PM, Theodore Dubois <tblodt(a)icloud.com> wrote: I CC’d him on the original email.
It would be neat if the wine program name could show up in Activity Monitor on OSX, so I can tell one wine process apart from another. There is a way to do it (http://stackoverflow.com/questions/4217947/setting-process-name-on-mac-os-x-...) but it requires undocumented APIs. (Damn.)
~Theodore
On Apr 6, 2016, at 3:35 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Ah, I see. Yeah, that's weird. Have you tried asking Chip directly? I'm CC'ing him.
-Ken
On Apr 6, 2016, at 4:53 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup.
~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
I know Crossover uses a custom loader for app bundles, have there been any problems there? ~Theodore
On Apr 26, 2016, at 1:35 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
I have both a custom wine loader and a hacked version of Wine, so I just reverted the patch.
It would be really nice to have this work for when I’m not using a custom loader. Maybe you could check if argv is in the malloc heap? Or maybe I could somehow indicate when calling wine_init that the argv is malloced?
~Theodore
On Apr 26, 2016, at 1:02 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
Sorry it took me so long to respond.
What this actually does is it changes the ps output so that the wine binary doesn't show up in the command line. Naturally, it relies on the argv block being one huge null-null-terminated list of strings (as most UNIX-like systems do, apparently). If you're not building your argv this way, then yes, wine will crash. Also, if wine's idea of the argv block isn't the one that the OS gave to main(), the ps output won't be affected.
The code that manipulates the argv block existed in the Linux case. I surmise, then, that we'd have this same problem there.
I'd kinda like to keep it, but then we'd need some way to detect that the argv we have isn't the block that was passed to main(), or isn't a null-null-terminated string list. (Or, maybe I can use _NSGetArgv() on Mac OS... but then all the other systems we support will still have this problem, and since you've written your own loader, we can't really rely on the order of arguments being "wine" "program" "...".) If you'd like me to just gut that piece of code, I'll be happy to write a patch to do that.
Chip
On Wed, Apr 6, 2016 at 4:03 PM, Theodore Dubois <tblodt(a)icloud.com> wrote: I CC’d him on the original email.
It would be neat if the wine program name could show up in Activity Monitor on OSX, so I can tell one wine process apart from another. There is a way to do it (http://stackoverflow.com/questions/4217947/setting-process-name-on-mac-os-x-...) but it requires undocumented APIs. (Damn.)
~Theodore
On Apr 6, 2016, at 3:35 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Ah, I see. Yeah, that's weird. Have you tried asking Chip directly? I'm CC'ing him.
-Ken
On Apr 6, 2016, at 4:53 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
Yes, I did. The problem is that the code assumes that argv[0] - argv[1] (= offset) is a meaningful quantity, which it isn’t because each string in the argv array came from strdup.
~Theodore
On Apr 6, 2016, at 2:14 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Did you allocate one more entry in your argv than is needed for actual arguments and put a terminating NULL? You're supposed to.
-Ken
On Apr 6, 2016, at 3:41 PM, Theodore Dubois <tblodt(a)icloud.com> wrote:
This patch causes a segfault in the event that __wine_main_argv is not the argv that got passed to main. This happened in a program I wrote that acts as a custom wine loader. It creates an argv by using malloc, then passes that to wine_init.
Why wouldn’t setprogname(argv[1]) be adequate?
~Theodore
> On Feb 19, 2016, at 2:33 PM, Charles Davis <cdavis5x(a)gmail.com> wrote: > > Signed-off-by: Charles Davis <cdavis5x(a)gmail.com>
On 26.04.2016 22:35, Theodore Dubois wrote:
I have both a custom wine loader and a hacked version of Wine, so I just reverted the patch.
It would be really nice to have this work for when I’m not using a custom loader. Maybe you could check if argv is in the malloc heap? Or maybe I could somehow indicate when calling wine_init that the argv is malloced?
~Theodore
May I ask why you are not just allocating the argv array in a way compatible to the OS? When you have full control in your custom loader, this should be trivial and the easiest way to fix it. Regards, Sebastian
Don’t know why I didn’t think of that. I’m going to give it a try and see if there’s any reason I can’t get it to work. ~Theodore
On Apr 26, 2016, at 1:38 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
On 26.04.2016 22:35, Theodore Dubois wrote:
I have both a custom wine loader and a hacked version of Wine, so I just reverted the patch.
It would be really nice to have this work for when I’m not using a custom loader. Maybe you could check if argv is in the malloc heap? Or maybe I could somehow indicate when calling wine_init that the argv is malloced?
~Theodore
May I ask why you are not just allocating the argv array in a way compatible to the OS? When you have full control in your custom loader, this should be trivial and the easiest way to fix it.
Regards, Sebastian
On Apr 26, 2016, at 3:38 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote:
On 26.04.2016 22:35, Theodore Dubois wrote:
I have both a custom wine loader and a hacked version of Wine, so I just reverted the patch.
It would be really nice to have this work for when I’m not using a custom loader. Maybe you could check if argv is in the malloc heap? Or maybe I could somehow indicate when calling wine_init that the argv is malloced?
~Theodore
May I ask why you are not just allocating the argv array in a way compatible to the OS? When you have full control in your custom loader, this should be trivial and the easiest way to fix it.
Forgive me for reviving an old thread, but how was it determined that allocating all of the strings pointed to by argv as a single buffer is what the OS does? This does not seem like a portable assumption. Also, why should set_process_name() shift the strings up within the (supposedly contiguous) string buffer? For the HAVE_SETPROCTITLE, it doesn't do that. Prior to Chip's commit, it didn't do that on macOS. So, Theodore's original question, "Why wouldn’t setprogname(argv[1]) be adequate?", is pretty close. It should have been, "Why wouldn’t setprogname(argv[1]) followed by shifting the elements of argv up one be adequate?" I'm not even sure why the string buffer is shifted for Linux. It looks like it's leftover logic that's no longer relevant (having to do with vdm?). I'll also note that Chip's commit broke things for the case where none of HAVE_SETPROCTITLE, HAVE_SETPROGNAME, or HAVE_PRCTL is defined. In that case, nothing shifts the elements of argv up one anymore. It used to. -Ken
On Jul 7, 2016, at 6:02 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
Also, why should set_process_name() shift the strings up within the (supposedly contiguous) string buffer? For the HAVE_SETPROCTITLE, it doesn't do that. Prior to Chip's commit, it didn't do that on macOS. So, Theodore's original question, "Why wouldn’t setprogname(argv[1]) be adequate?", is pretty close. It should have been, "Why wouldn’t setprogname(argv[1]) followed by shifting the elements of argv up one be adequate?"
I'm not even sure why the string buffer is shifted for Linux. It looks like it's leftover logic that's no longer relevant (having to do with vdm?).
OK, I have figured out what I was missing. Much to my surprise, modifying the argv strings buffer is actually the means by which the command shown by, for example, ps is modified. The prctl() call on Linux affects a different value (the "comm" field rather than "command"). I have submitted a patch to address the other shortcomings of the current code. -Ken
participants (4)
-
Charles Davis -
Ken Thomases -
Sebastian Lackner -
Theodore Dubois