Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

limit_handles breaks exec() error reporting on POSIX #202

Closed
tomix86 opened this issue Mar 16, 2021 · 2 comments
Closed

limit_handles breaks exec() error reporting on POSIX #202

tomix86 opened this issue Mar 16, 2021 · 2 comments

Comments

@tomix86
Copy link

tomix86 commented Mar 16, 2021

Using limit_handles on POSIX platforms causes internal logic for exec() error reporting to break.
The root cause is that foreach_used_handle on POSIX does not account for executor::_pipe_sink, which is needed for error reporting from child to parent.

A minimal reproducer:

#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include <boost/process.hpp>
#include <boost/process/extend.hpp>

namespace bp = boost::process;

TEST(BoostProcessTest, limit_handles_breaks_exec_error_handling) {
	{
		std::error_code ec;
		bp::child{bp::exe = "some_nonexistent_path", ec}.wait();
		EXPECT_EQ(std::errc::no_such_file_or_directory, ec);
	}

	{
		std::error_code ec;
		bp::child{bp::exe = "some_nonexistent_path", ec, bp::limit_handles}.wait();
		// EXPECT_EQ(std::errc::no_such_file_or_directory, ec);
		EXPECT_EQ(std::error_code{}, ec); // Boost.Process bug
	}
}

int main(int argc, char** argv) {
	::testing::InitGoogleTest(&argc, argv);
	return RUN_ALL_TESTS();
}
@sfc-gh-hyu
Copy link

Got the same error, is there any update on this? Or at least small patch on how to fix this?

@dkl
Copy link
Contributor

dkl commented Feb 20, 2024

Testing with boost-process 1.84.0, this issue still seems to exist. Commit 1a1d677 added executor::get_used_handles(), but it doesn't work, because _pipe_sink is assigned after the on_setup() call, so it's missed and closed anyways.

#include <boost/process/handles.hpp>
#include <boost/process/system.hpp>
int main() {
	// with limit_handles: no exception thrown, but should be
	// without limit_handles: throws exception as expected
	boost::process::system("/does/not/exist", boost::process::limit_handles);
	return 0;
}

dkl added a commit to dkl/boost-process that referenced this issue Apr 17, 2024
_pipe_sink was assigned after call_on_setup(), after limit_fd_::on_setup(),
but this was too late. It must be assigned earlier so that
executor::get_used_handles() can see it and prevent limit_handles from
closing the internal pipe for passing exec() errors from child to parent.

Fixes: 1a1d677
Closes: boostorg#202
Signed-off-by: Daniel Klauer <[email protected]>
klemens-morgenstern pushed a commit that referenced this issue Jun 4, 2024
_pipe_sink was assigned after call_on_setup(), after limit_fd_::on_setup(),
but this was too late. It must be assigned earlier so that
executor::get_used_handles() can see it and prevent limit_handles from
closing the internal pipe for passing exec() errors from child to parent.

Fixes: 1a1d677
Closes: #202
Signed-off-by: Daniel Klauer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants