libsubprocess: do not allow short writes with flux_subprocess_write()
#5548
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: The flux_subprocess_write() function may return a short write if all the data being written does not fit in the destination buffer, but short writes are tricky to handle and mostly cause a fatal error in current code anyway. In some places, short writes are not detected and would result in unpredictable behavior. Avoid short writes in flux_subprocess_write() by checking the available buffer space before writing. Return -1 with errno set to ENOSPC if there is not enough available space to write the entire buffer.
Problem: No tests in the libsubprocess unit tests ensure that flux_subprocess_write() returns -1 wih errno == ENOSPC when the requested data cannot be buffered. Add a test that creates a subprocess with a very small buffer and ensures that a write exceeding that buffer space returns -1 with ENOSPC (instead of a short write).
Problem: In the libsubprocess server code, the result of flux_subprocess_write() is checked for a short write, but this is no longer a possible result. Remove the unnecessary check.
Problem: The rexec and rexec_getline tests in the testsuite check for a short write from flux_subprocess_write(), but this is no longer possible. Remove the special case for a short write return code from flux_subprocess_write() in these test programs.
grondo
force-pushed
the
subprocess-no-short-writes
branch
from
fbf1832 to
2e8a643
Compare
Codecov Report
@@ Coverage Diff @@
## master #5548 +/- ##
==========================================
+ Coverage 83.42% 83.45% +0.02%
==========================================
Files 480 487 +7
Lines 80730 82286 +1556
==========================================
+ Hits 67347 68669 +1322
- Misses 13383 13617 +234
|
garlick
approved these changes
Nov 9, 2023
|
This solves half the problem. I'll have another PR with the other half tomorrow. (Struggling a bit with how to test...) I'll set MWP here though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was pulled off some other work.
Currently,
flux_subprocess_write()writes any bytes it can to the destination buffer, which could result in a short write (return code <len). However, a check for a short write is not done is most places (a check forrc < 0seems to be the common case), and where it is checked it is immediately promoted to a fatal error. In some cases (such as the job-ingest pipeline) a short write can be dangerous since it isn't recovered and writing only part of a line "hangs" the protocol.In the interest of simplicity, add a check for the available buffer space in
flux_subprocess_write()and return immediately with return code-1anderrnoset toENOSPCif there is not enough space for all the data. Update any callers that attempt to handle a short write as a special case.In the future we'll need a more sophisticated buffered write function anyway that can handle flow control and/or a chain of buffers instead of adding short-write handling at each caller (e.g. see #5542)