Work around popen not being thread safe on MacOS

[Thomas1664]

Related microsoft/vcpkg#26651

vcpkg occasionally crashed at decompress_in_parallel > cmd_execute_and_capture_output_parallel because threads that are launched by cmd_execute_and_capture_output_parallel were writing to the result vector without locking the vector.

Not a problem because they access different indexes of the vector and don't cause a resize.

@omartijn Thanks for providing the debug information.

max_concurrency()

get_concurrency() returned int which didn't make any sense because there can't be -1 threads.

[Thomas1664]

Who says a user puts only numbers >= -1 in here?

[Thomas1664]

@BillyONeal Should we throw if VCPKG_MAX_CONCURRENCY is invalid or should we simply continue with std::thread::hardware_concurrency()

[Thomas1664]

@omartijn Gotcha!

I created a unit test that executes the command echo a where a is a string of the char a with a length that matches the current index, i.e. `` for index 0, a for index 1, `aa` for index 2 and so on.

Here are the results:

...
Index: 36, length: 36
Index: 37, length: 37
Index: 38, length: 18446744073709551615

The problem is that in random positions the string returned from the child process is missing a \0 which leads to a string length of SIZE_T_MAX. I subtracted 1 in my test to align with the current index.

The segfault comes into place when someone tries to read at index > 38 in this case.

[autoantwort]

@Thomas1664 Can the change for the mutex around popen be extracted to another PR? Its very annoying when vcpkg crashes and I don't want to wait longer :)

I think the issue you found with asan already fixes the crash that I tried to fix with this PR?

I fixed another crash, but there are multiple ones: #695 (comment)

[autoantwort]

I only see this:

[Thomas1664]

Never mind, the crash isn't fixed

[Thomas1664]

@autoantwort If unit tests succeed on OSX in CI and the message Mutex enabled is printed to the console, could you please run the unit tests from this branch on your Mac (IIRC you do have one) 100-1000 times and if it crashes try setting the buffer to 1023 and try again? Seems like CI can't reliably test this.

EDIT: Never mind, I noticed that the debug message I added wasn't printed.

[autoantwort]

EDIT: Never mind, I noticed that the debug message I added wasn't printed.

Because you have to pass --debug but you can't so that for the vcpkg-test executable

[autoantwort]

Seems that it fails with std::async because that also calls popen (or pthread_create)

[Thomas1664]

Seems that it fails with std::async because that also calls popen (or pthread_create)

@autoantwort Although it shouldn't matter but did you also try to set the buffer back to 1023 at the line you commented about #695 (comment) ?

[autoantwort]

Finally found it. The pclose call also has to be protected by the mutex.

[autoantwort]

All the errors I got happens before the fgets line. This has nothing to do with the buffer size. All got all errors while the buffer size was 1023.

[autoantwort]

Apply this patch to fix the pipeline:

diff --git a/src/vcpkg/base/system.process.cpp b/src/vcpkg/base/system.process.cpp
index c6eb7820..c00713c8 100644
--- a/src/vcpkg/base/system.process.cpp
+++ b/src/vcpkg/base/system.process.cpp
@@ -960,7 +960,15 @@ namespace vcpkg
             return format_system_error_message("feof", errno);
         }
 
-        int ec = pclose(pipe);
+        int ec;
+        {
+#if defined(__APPLE__)
+            // `pclose` sometimes returns 127 on OSX when executed in parallel.
+            // Related: https://github.com/microsoft/vcpkg-tool/pull/695#discussion_r973364608
+            std::lock_guard guard(mtx);
+#endif
+            ec = pclose(pipe);
+        }
         if (WIFEXITED(ec))
         {
             ec = WEXITSTATUS(ec);

[autoantwort]

I approve the macOS specific changes

[Thomas1664]

Finally found it. The pclose call also has to be protected by the mutex.

@autoantwort Thanks a lot for your help!

[Thomas1664]

It is even documented that our use of popen was wrong: Although popen is thread safe, "It is even possible that calling MT-Safe functions in sequence does not yield an MT-Safe combination." (source: https://www.man7.org/linux/man-pages/man7/attributes.7.html ). Note that we might call this function in sequence across multiple threads:

vcpkg-tool/src/vcpkg/base/system.process.cpp

Lines 485 to 491 in c0e76df

	auto work = [&]() {
	std::size_t item;
	while (item = work_item.fetch_add(1), item < cmd_lines.size())
	{
	res[item] = cmd_execute_and_capture_output(cmd_lines[item], wd, env);
	}
	};

[ras0219-msft]

It is even documented that our use of popen was wrong: Although popen is thread safe, "It is even possible that calling MT-Safe functions in sequence does not yield an MT-Safe combination." (source: https://www.man7.org/linux/man-pages/man7/attributes.7.html ).

I think adding more context to the quote makes more sense:

For example, having a thread call two MT-
Safe functions one right after the other does not
guarantee behavior equivalent to atomic execution of a
combination of both functions, since concurrent calls in
other threads may interfere in a destructive way.

My interpretation is that this is restating how

int a = x.fetch_add(1);
int b = x.fetch_add(1);

doesn't guarantee b - a == 1 -- another thread could have modified x between. Thus, our use should still be permissible.

But all of this is largely philosophical. Given the evidence in this thread, I think adding a lock around popen and pclose has presented as a 100% fix to the issue (with a small sample size).

[BillyONeal]

But all of this is largely philosophical. Given the evidence in this thread, I think adding a lock around popen and pclose has presented as a 100% fix to the issue (with a small sample size).

Does that mean you are OK with this change? Note that it is currently unable to merge because you are marked 'request changes'

[BillyONeal]

Thanks :)

[autoantwort]

Whoop whoop. Thank you! ❤️