On 12.07.2016 06:44, Ken Thomases wrote:
This fixes several problems with the code:
- The code had been assuming that the argument strings pointed to by the argv array are contiguous iff certain process-name-setting functions are available. This doesn't seem reliable. Instead, test if it's true and shift the strings if so.
Thanks for working on this, looks indeed like a nice improvement.
However, setproctitle() is specifically documented as a preferred alternative to the technique of overwriting the arg strings, so don't shift the strings if that's available.
Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way.
Use the last path component, recognizing backslash as a path separator, for setprogname() in addition to prctl(). First, setprogname() is documented as searching for the last component itself, but it doesn't understand Windows- style paths, so we need to help it. Second, on some platforms (e.g. macOS), setprogname(), like prctl(), has a fairly small internal length limit (e.g. 32 characters). So, concentrate on the most meaningful part of the path.
Remove argv[0] from argv whether or not there are any process-name-setting functions available. This is necessary for the proper functioning of Wine, so it must be done on all platforms. This part of the logic was lost with commit 5a4576ee0.
Call all available process-name-setting functions instead of treating them as mutually exclusive alternatives. This is also logic that was lost with commit 5a4576ee0.
Signed-off-by: Ken Thomases ken@codeweavers.com
dlls/kernel32/process.c | 63 +++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 26 deletions(-)
On Jul 12, 2016, at 2:45 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 12.07.2016 06:44, Ken Thomases wrote:
This fixes several problems with the code:
- The code had been assuming that the argument strings pointed to by the argv
array are contiguous iff certain process-name-setting functions are available. This doesn't seem reliable. Instead, test if it's true and shift the strings if so.
Thanks for working on this, looks indeed like a nice improvement.
You're welcome and thanks. :)
However, setproctitle() is specifically documented as a preferred alternative to the technique of overwriting the arg strings, so don't shift the strings if that's available.
Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way.
Well, the only reason we remove the directory component for the other calls is because they only allow limited storage. I'm not aware of a similar limit for setproctitle().
Also, setproctitle() is theoretically more similar to the shifting of the argument strings than to setprogname() or prctl(PR_SET_NAME). And we don't make any attempt to remove the directory component for the string shifting. (I.e. "ps" will show the full command line, potentially including Windows-style .exe path.)
Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings.
-Ken
On 13.07.2016 20:20, Ken Thomases wrote:
However, setproctitle() is specifically documented as a preferred alternative to the technique of overwriting the arg strings, so don't shift the strings if that's available.
Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way.
Well, the only reason we remove the directory component for the other calls is because they only allow limited storage. I'm not aware of a similar limit for setproctitle().
Also, setproctitle() is theoretically more similar to the shifting of the argument strings than to setprogname() or prctl(PR_SET_NAME). And we don't make any attempt to remove the directory component for the string shifting. (I.e. "ps" will show the full command line, potentially including Windows-style .exe path.)
Well, its still very different compared to shifting because all the following arguments are stripped away. I did some quick tests and it seems like there are also methods which do preserve the additional arguments in the "ps" output:
* Either a direct sysctl invocation as done in the setproctitle() implementation itself.
* Or, first shift the arguments (both methods work), and then call: setproctitle("some string"); setproctitle(NULL); The second call restores the process title as seen by the app.
Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings.
I'm not sure if two #ifdefs is actually that bad, it probably should depend on the operating system anyway. On systems where it doesn't make a difference (like FreeBSD, ...) this step can always be safely skipped.
-Ken
On 13.07.2016 21:25, Sebastian Lackner wrote:
On 13.07.2016 20:20, Ken Thomases wrote:
However, setproctitle() is specifically documented as a preferred alternative to the technique of overwriting the arg strings, so don't shift the strings if that's available.
Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way.
Well, the only reason we remove the directory component for the other calls is because they only allow limited storage. I'm not aware of a similar limit for setproctitle().
Also, setproctitle() is theoretically more similar to the shifting of the argument strings than to setprogname() or prctl(PR_SET_NAME). And we don't make any attempt to remove the directory component for the string shifting. (I.e. "ps" will show the full command line, potentially including Windows-style .exe path.)
Well, its still very different compared to shifting because all the following arguments are stripped away. I did some quick tests and it seems like there are also methods which do preserve the additional arguments in the "ps" output:
Either a direct sysctl invocation as done in the setproctitle() implementation itself.
Or, first shift the arguments (both methods work), and then call: setproctitle("some string"); setproctitle(NULL); The second call restores the process title as seen by the app.
Actually it looks like we can get the same result just by concatenating all arguments on the Wine side. The separation of the arguments is lost in the "ps" output anyway. Depending on how you want to proceed I'll probably send a patch after you are done with the cleanup. ;)
On Jul 13, 2016, at 2:37 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 13.07.2016 21:25, Sebastian Lackner wrote:
On 13.07.2016 20:20, Ken Thomases wrote:
However, setproctitle() is specifically documented as a preferred alternative to the technique of overwriting the arg strings, so don't shift the strings if that's available.
Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way.
Well, the only reason we remove the directory component for the other calls is because they only allow limited storage. I'm not aware of a similar limit for setproctitle().
Also, setproctitle() is theoretically more similar to the shifting of the argument strings than to setprogname() or prctl(PR_SET_NAME). And we don't make any attempt to remove the directory component for the string shifting. (I.e. "ps" will show the full command line, potentially including Windows-style .exe path.)
Well, its still very different compared to shifting because all the following arguments are stripped away. I did some quick tests and it seems like there are also methods which do preserve the additional arguments in the "ps" output:
- Either a direct sysctl invocation as done in the setproctitle()
implementation itself.
- Or, first shift the arguments (both methods work), and then call:
setproctitle("some string"); setproctitle(NULL); The second call restores the process title as seen by the app.
Actually it looks like we can get the same result just by concatenating all arguments on the Wine side. The separation of the arguments is lost in the "ps" output anyway. Depending on how you want to proceed I'll probably send a patch after you are done with the cleanup. ;)
I'm happy to have my current patch committed and you can submit a patch to pass the concatenation of the args to setproctitle() if you like. :)
-Ken
On Jul 13, 2016, at 2:25 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 13.07.2016 20:20, Ken Thomases wrote:
Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings.
I'm not sure if two #ifdefs is actually that bad, it probably should depend on the operating system anyway.
By "it" you mean the decision to shift the strings? Why should that depend on the OS? What reliable way do we have to detect if the OS works that way other than the strings actually being contiguous?
On systems where it doesn't make a difference (like FreeBSD, ...) this step can always be safely skipped.
By "systems where it doesn't make a difference", you mean those that have setproctitle()? By "this step" you mean shifting the strings? That's what my patch already does.
In other words, I don't understand what you're getting at with this paragraph.
-Ken
On 13.07.2016 21:52, Ken Thomases wrote:
On Jul 13, 2016, at 2:25 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 13.07.2016 20:20, Ken Thomases wrote:
Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings.
I'm not sure if two #ifdefs is actually that bad, it probably should depend on the operating system anyway.
By "it" you mean the decision to shift the strings? Why should that depend on the OS? What reliable way do we have to detect if the OS works that way other than the strings actually being contiguous?
Please correct me if I'm wrong, but to my knowledge, the "ugly" shift string method which depends on the exact memory layout only makes sense on Linux. On all other operating systems the cmdline is obtained in a different way and it does not matter what is stored in the process.
On systems where it doesn't make a difference (like FreeBSD, ...) this step can always be safely skipped.
By "systems where it doesn't make a difference", you mean those that have setproctitle()? By "this step" you mean shifting the strings? That's what my patch already does.
What I mean is that checking the operating system might be better than checking if specific functions are available. To explain it with some pseudo code, I was thinking about something like this:
--- snip --- #ifdef __linux__ // shift_strings check
if (shift_strings) { // ... } else #endif { // fallback method }
#ifdef HAVE_SETPROCTITLE // ... #endif #ifdef HAVE_SETPROGNAME // ... #endif #ifdef HAVE_PRCTL // ... #endif --- snip ---
Regards, Sebastian
On Jul 13, 2016, at 3:13 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 13.07.2016 21:52, Ken Thomases wrote:
On Jul 13, 2016, at 2:25 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 13.07.2016 20:20, Ken Thomases wrote:
Moving the code to the bottom could be done, although it would mean two #ifdef HAVE_SETPROCTITLE blocks, one to set shift_strings and the other to actually make the call. The reason is that the shifting has to happen before the call to setprogname() because it's documented to keep the pointer that's passed in. We can't pass it (a substring of) argv[1] and then shift the strings.
I'm not sure if two #ifdefs is actually that bad, it probably should depend on the operating system anyway.
By "it" you mean the decision to shift the strings? Why should that depend on the OS? What reliable way do we have to detect if the OS works that way other than the strings actually being contiguous?
Please correct me if I'm wrong, but to my knowledge, the "ugly" shift string method which depends on the exact memory layout only makes sense on Linux. On all other operating systems the cmdline is obtained in a different way and it does not matter what is stored in the process.
That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output.
The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3
-Ken
On 14.07.2016 00:46, Ken Thomases wrote:
Please correct me if I'm wrong, but to my knowledge, the "ugly" shift string method which depends on the exact memory layout only makes sense on Linux. On all other operating systems the cmdline is obtained in a different way and it does not matter what is stored in the process.
That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output.
The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3
-Ken
Okay, if its unclear what else we might break then its probably better to keep it in the version you already have. Thanks for the explanation. ;)
Regards, Sebastian
On Jul 13, 2016, at 8:36 PM, Sebastian Lackner sebastian@fds-team.de wrote:
On 14.07.2016 00:46, Ken Thomases wrote:
Please correct me if I'm wrong, but to my knowledge, the "ugly" shift string method which depends on the exact memory layout only makes sense on Linux. On all other operating systems the cmdline is obtained in a different way and it does not matter what is stored in the process.
That's what I thought at first, too, but no. Shifting the strings is what affects the output of ps on macOS, too. The call to setprogname() affects some other process metadata query APIs (e.g. libproc), but I'm not aware of built-in commands which demonstrate their output.
The setproctitle() docs I've found online suggest that modifying argv (at least, argv[0]) works on FreeBSD, too. It's just that setproctitle() is preferred. https://www.freebsd.org/cgi/man.cgi?query=setproctitle&sektion=3
-Ken
Okay, if its unclear what else we might break then its probably better to keep it in the version you already have. Thanks for the explanation. ;)
You're welcome. Thank you for reviewing. :)
-Ken
-
Ken Thomases -
Sebastian Lackner