Fix bug recently introduced in run_program().

[davidpanderson]

In the Windows variant, the "file" argument was being ignored (??).

[AenBleidd]

I think @JuhaSointusalo should review this PR because he made changes in this function previously

[RichardHaselgrove]

I've verified the problem reported in #2739 across a LAN - no VM involved. No errors were logged in C:\Users\Richard Haselgrove\AppData\Roaming\BOINC.

Searching Git, run_program() is called from 7 different files:

tools/updater.cpp
clientscr/screensaver.cpp
client/auto_update.cpp
client/gpu_detect.cpp
clientgui/BOINCClientManager.cpp
clientgui/MainDocument.cpp
clientgui/AdvancedFrame.cpp

In #2713, @JuhaSointusalo only changed the call in client/gpu_detect.cpp: I'm nervous that simply reverting the change in lib/util.cpp will move the bug from one place to another, rather than fixing it. I agree that Juha should review it.

[ChristianBeer]

Just in case it was missed. This change was already added to the 7.14 release branch with d5e6d95 so any changes here need be be made to the release branch too.

[JuhaSointusalo]

run_program() was changed in 6db39da with the following commit message:

lib: change run_program() to use $PATH

GPU detection tries to launch a copy of the client using full path to
the client executable but getting the full path may fail. In this case
the code falls back to using argv[0] but argv[0] doesn't always contain
path to the client. This may happen if the client was started from shell
and the shell used $PATH to find the client executable.

Change run_program() to search $PATH for the program file if the file
name doesn't include path.

CreateProcess() is declared like this:

BOOL CreateProcess(LPCTSTR lpApplicationName, LPTSTR lpCommandLine, ...)

The relevant parts from docs:

lpApplicationName
The string can specify the full path and file name of the module to execute or it can specify a partial name. In the case of a partial name, the function uses the current drive and current directory to complete the specification. The function will not use the search path. ...

lpCommandLine
If lpApplicationName is NULL, the first white space–delimited token of the command line specifies the module name. ... If the file name does not contain a directory path, the system searches for the executable file in the following sequence:

The directory from which the application loaded.
The current directory for the parent process.
The 32-bit Windows system directory. Use the GetSystemDirectory function to get the path of this directory.
The 16-bit Windows system directory. There is no function that obtains the path of this directory, but it is searched. The name of this directory is System.
The Windows directory. Use the GetWindowsDirectory function to get the path of this directory.
The directories that are listed in the PATH environment variable. Note that this function does not search the per-application path specified by the App Paths registry key. To include this per-application path in the search sequence, use the ShellExecute function.

So, in order to have CreateProcess() search $PATH lpApplicationName must be NULL.

run_program() tries to provide a fork() + execve() compatible interface to systems that don't have them. Important part of the interface is how arguments are passed. From exec*() POSIX page:

The argument argv is an array of character pointers to null-terminated strings. The application shall ensure that the last member of this array is a null pointer. These strings shall constitute the argument list available to the new process image. The value in argv[0] should point to a filename string that is associated with the process being started by one of the exec functions.

lpCommandLine is constructed in run_program() by concatenating elements of the argv vector together. If argv[0] points to a filename string then that string becomes the first token in lpCommandLine and CreateProcess() can find the program file just fine.

run_program() doesn't work for launching graphics apps because we have code passing in who knows what in argv in calls to run_program().

The changes I did are not exactly correct. My changes expect argv[0] to be full path to the program file but POSIX says it should be only the filename. Even though in practise its a matter of taste which one argv[0] is the code should be prepared to handle only filename in argv[0].

The changes in this PR are not correct. I'll make a PR that fixes things right way later this week.

---

As for the PR description.

I put in some serious effort in writing good commit messages that tell not only what was changed but WHY the change was made. And then I get to read this

(??)

as well as the rubbish commit message that tells nothing about what motivated reverting my changes. I'm not too happy about that.

And I see that this change was already pushed to 7.14 branch. WTF?!

[AenBleidd]

And I see that this change was already pushed to 7.14 branch. WTF?!

Totally agree. From my prospective, changes to release branch must never be pushed before being reviewed, approved and merged to master branch.

[Uplinger]

@davidpanderson

I'm with @JuhaSointusalo and @AenBleidd on this being included in the current release branch before being merged into master. I think that your change needs to be backed out of the current 7.14 release branch.

Note: From the BOINC Flow document that was recently approved: https://github.com/BOINC/boinc-policy/blob/master/Development_Documents/BOINC_Flow.md

"Any code that is added to a release branch after it is created should be a cherry-pick of a merge commit that is present on the master branch. "

Since the 7.14 branch was already created, the only code changes that can be added to that release branch are ones that are cherry-picked from the master branch. Thus, the code change you made for this needs to be reverted.

Thanks,
-Keith Uplinger

[CharlieFenton]

The changes in this PR are not correct. I'll make a PR that fixes things right way later this week.

Please let's put this in perspective. Remember that there are serious issues on the Mac with 7.12.1 due to Apple's release of OS 10.14 Mojave. I have not released a hot fix 7.12.2 in the expectation that 7.14 will be released very soon. At this point, only critical bug fixes should go into the 7.14 branch.

I am currently working on a fix for a security issue which I hope to have ready within 2 days.

Juha's commit 6db39da broke several areas of important functionality. Why can't the same thing be accomplished by modifying the way COPROCS::launch_child_process_to_detect_gpus()calls run_program, by passing NULL instead of client_path? This would eliminate the need to change run_program() itself and all the other places it is called.

The more extensive changes could then wait for 7.16 so there will be adequate time to test them.

I'm very concerned that creating even more changes to fix 6db39da will create the potential for further bugs, especially on platforms other than MSWindows. If we have to wait until later this week for his changes, it will further delay the testing we need. At the very least, any changes to the calls to run_program() must be tested on a *NIX system.

[CharlieFenton]

I have no problem with changing execv() to execvp() in run_program() if that is necessary to fix the GPU detection.

[RichardHaselgrove]

I'm afraid to say I started this whole sorry fandango by passing on user reports in, first, #2029, and when the fix for that proved to be flawed, again in #2657.

I think we've now reached the point where we should revert #2713 and veto this (dependent) PR. Although I'm a great believer in responding to user reports in the next available client release, this issue is not a blocker and there are more important issues for v7.14

That means reverting the associated commits 388dcea and d5e6d95 too.

Then, we can all take a deep breath, work out what went wrong, and fix the problem fully and properly in v7.16 (or v8.0, if we go there instead), with everyone's informed consent beforehand.

[TheAspens]

I've created pull request #2747 as a proposal to allow us to move forward (it is based on @RichardHaselgrove's suggestion).

@davidpanderson and @JuhaSointusalo - can you review #2747 and see if it makes sense and if you agree with the approach?

[AenBleidd]

I think this should be closed without merging.

CC: @TheAspens, @davidpanderson

[CharlieFenton]

I agree, but the bug introduced by Juha's commit 6db39da [which ignored the second argument file in run_program()] is still in master. So we still need to port to master the commit d5e6d95 @davidpanderson made in 7.14 (admittedly without a PR) to revert just the 2 lines in run_program(). Otherwise the Show Graphics and screensaver won't work on MS Windows.

[CharlieFenton]

After writing my previous comment, I see that @JuhaSointusalo has said his PR #2748 is ready for review. I have not reviewed it, but that PR is designed to fix Show Graphics and screensaver on MS Windows. So I withdraw my previous comment.

[AenBleidd]

I'm closing this PR without merging if favor of issue fix in #2748