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

Fix a race condition in process.run_duplicate_streams #186

Merged
merged 3 commits into from
Oct 15, 2019

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Oct 1, 2019

Fixes issue #:
in-toto/in-toto#282

Description of the changes being introduced by the pull request:
This PR fixes a race condition in a standard stream duplicating loop in process.run_duplicate_streams, where the subprocess would write to its standard streams after the last read in the parent process, and then terminate before another iteration would be started, and therefor leaving the last parts of the streams unread.

Also adds a note about buffered outputs in child process to the function's docstring.

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

This commit fixes a race condition in a standard stream duplicating
loop in `process.run_duplicate_streams`, where the subprocess would
write to its standard streams after the last read in the parent
process, and then terminate before another iteration would be
started, and therefor leaving the last parts of the streams unread.

This fix includes a last post-loop read, which can only take place
after the process has terminated, and no longer writes to its
streams.
@coveralls
Copy link

coveralls commented Oct 1, 2019

Coverage Status

Coverage increased (+0.0001%) to 99.875% when pulling 3cec858 on fix-duplicate-streams into 8df13b0 on master.

This fixes a potential subversion of the timeout parameter
in `process.run_duplicate_streams`.

That is, if the child process writes to much and too fast to its
standard streams, the parent process will take too long (or even
indefinitely) to read and then write the contents to its own
standard streams, before it e.g. checks whether the process should
timeout.

This change adds a size parameter to the relevant read functions
to prevent above.
@lukpueh
Copy link
Member Author

lukpueh commented Oct 1, 2019

3cec858 fixes another issue in process.run_duplicate_streams (see commit message and code comment for details). Kudos to @aaaaalbert for reminding me to look closer)

lukpueh added a commit to in-toto/in-toto that referenced this pull request Oct 1, 2019
The added patches fix two race conditions in in-toto's
process module.
They will be fixed upstream, but not in in-toto but in
securesystemslib, where the process module has been moved
to.

See secure-systems-lab/securesystemslib#186

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh
Copy link
Member Author

lukpueh commented Oct 15, 2019

Many thanks, @adityasaky!

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