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

Allow setting stdout_file in non-fork launcher #2127

Merged
merged 4 commits into from
May 2, 2024

Conversation

vringar
Copy link
Contributor

@vringar vringar commented Apr 30, 2024

Previously setting the stdout_file in the launcher, while not forking would only result in the stdout being forwarded from the child processes to the parent process, but not written into the specified location.

This PR changes this behaviour to make the parent and all children write into the log file. It also hides the file in non-unix builds. This means, unless debug output is set, on non-unix platforms the stdio from child processes is hidden.

Discovered while debugging #2111

@vringar vringar changed the title Fix/launcher fix(launcher.rs): hide file output behind appropriate feature flag Apr 30, 2024
@vringar vringar marked this pull request as draft April 30, 2024 17:41
@vringar
Copy link
Contributor Author

vringar commented Apr 30, 2024

Still needs some work because the field is accessed both in windows and unix code.
I think it might be easier to just implement the functionality for the non-windows, non-forking code, because there is already a beginning of an implementation there

let stdio = if self.stdout_file.is_some() {
Stdio::inherit()
} else {
Stdio::null()
};

@tokatoka
Copy link
Member

yes windows should not access it. it cannot fork

@domenukk
Copy link
Member

Couldn't we make this work on Windows/non-fork by just (not) closing the output?

@vringar
Copy link
Contributor Author

vringar commented May 2, 2024

Couldn't we make this work on Windows/non-fork by just (not) closing the output?

We'd need to redirect stdout/stderr to the file first, and the way that is currently implemented is by dup2 the file descriptor, which afaik wouldn't work on Windows.
So my current idea is to support non-fork but still hide the fields on non-unix.

@vringar vringar changed the title fix(launcher.rs): hide file output behind appropriate feature flag Allow setting stdout_file in non-fork launcher May 2, 2024
@vringar vringar marked this pull request as ready for review May 2, 2024 11:12
@vringar
Copy link
Contributor Author

vringar commented May 2, 2024

I just verified that this now creates the file and writes all output to it, as expected on Linux. Idk, if the dup2 call is available on all unix platforms, but I copied the #cfg that was hiding the function, so it should be fine

@domenukk
Copy link
Member

domenukk commented May 2, 2024

Neat! :)

@domenukk domenukk merged commit ec944a0 into AFLplusplus:main May 2, 2024
100 checks passed
@vringar vringar deleted the fix/launcher branch May 2, 2024 14:31
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

Successfully merging this pull request may close these issues.

3 participants