Report the storage shortage in the env-var path instead of resetting silently - #167
Merged
Merged
Conversation
…silently
Before libc370#82 a shortage inside the request pipeline abended S878,
try(serve_client) caught it and logged HTTPD062E, and the connection
closed cleanly. Now http_set_env() just returns -1, every caller does
goto failed -> CSTATE_RESET, and the client sees a connection reset
with no response and nothing in the log -- indistinguishable from a
malformed request. Observability got worse for this path.
Report it at httpsenv(), the one choke point every caller passes
through and the last place that still knows which variable failed --
the reset labels in httpin.c and httppars.c are shared with socket
errors and malformed-request paths, so a message there could only
guess. httpnenv() now sets errno so the two rejects are told apart:
HTTPD905E for a variable over the sanity limit, HTTPD904E for an
actual shortage.
Two unchecked return values in the same path, both from the issue:
- httppc.c dispatched the CGI even when SCRIPT_FILENAME could not be
set, which runs it against no file at all; it now answers 500.
- parse_cookies() ignored set_cookie()'s rc in a loop, so a request
with 20 cookies would emit 20 messages instead of one. It now
stops at the first failure, which is what bounds the new message
to once per request -- the same cadence HTTPD062E had.
parse_cookies() also returned ENOMEM (12) where every other setter in
this path returns -1; normalized, callers only test truthiness.
TSTNENV covers the half that needs no storage shortage to reach: an
oversized name+value is rejected with E2BIG and one byte under the
limit still succeeds. Both assertions fail against the unfixed
httpnenv.c.
Fixes #162
Contributor
Author
|
Checked the one thing this design could get wrong: does the errno set in Yes, and the vector is not the boundary it looks like:
Also deliberate, not incidental: |
This file contains hidden or 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
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.
Fixes #162
The regression
Before libc370#82 a storage shortage inside the request pipeline abended S878;
try(serve_client, ...)caught it, loggedHTTPD062E, reset busy and closedcleanly. Now the same shortage makes
http_set_env()return -1 →goto failed→
CSTATE_RESET: a connection reset with no response and no log line,indistinguishable from a malformed request. Observability got worse.
Where the message goes, and why not at the reset
Not at the
failed:labels. Both of them (httpin.c:183,httppars.c:142) areshared with socket errors,
strtokreturning NULL and missing env vars, so amessage there could only guess at the cause — or need errno archaeology to tell
the cases apart.
httpsenv()is the one choke point every caller passes through —httpin,httppars,httpshen,httpsqen,httppc, and any CGI going through theHTTPX vector — and it is the last place that still knows which variable
failed. That is strictly more useful to an operator than "the connection was
reset".
http_new_env()has two rejects that both return NULL, so it now setserrnoto distinguish them:
HTTPD904E No storage for environment variable <name> client(<HTTPC>)HTTPD905E Environment variable <name> too large client(<HTTPC>)(the
client(...)operand is the HTTPC pointer, matching HTTPD900D's style, not the client IP)Its signature is unchanged, so the HTTPX vector entry is untouched.
Two unchecked return values in the same path
Both named in the issue:
httppc.cdispatched the CGI even whenSCRIPT_FILENAMEcould not beset, i.e. ran it against no file at all. It now answers 500.
parse_cookies()ignoredset_cookie()'s rc in a loop. Under ashortage a request with 20 cookies would have emitted 20 of the new messages.
It now stops at the first failure — that is what bounds the message to once
per request, the same cadence
HTTPD062Ehad.parse_cookies()also returnedENOMEM(12) where every other setter in thispath returns -1. Normalized; every caller only tests truthiness.
Testing
The oversize reject needs no storage shortage to reach, so it is tested.
TSTNENVgains three assertions: a name+value over the sanity limit is rejectedwith
E2BIG, and one byte under the limit still succeeds (so the reject cannotquietly swallow legitimate variables).
Run on the MVS target, and confirmed to actually discriminate — against the
unfixed
httpnenv.cit fails, with the fix it passes:The ENOMEM branch itself is inspection-only. It needs a real storage
shortage; there is no allocator interposition on this toolchain and building
injection into production code for it would be worse than saying so.
What was verified live instead is that every path this PR touches still behaves
on the non-OOM side — deployed, activated, restarted:
httpinenvGET /.dsrvhttpsqen/.dm?m=10&l=16&t=CVTPTRparse_cookies/set_cookie/zosmf/restfiles/ds?dslevel=…httppcSCRIPT_FILENAME*.httpdsrvGARBAGE\r\n\r\nThe
SCRIPT_FILENAMEbranch only runs for extension-based routes, of which theproduction Parmlib has none — so it was exercised through a temporary alternate
member (
MOD=HTTPDSRV→ derived*.httpdsrv), started withS HTTPD,M=…soHTTPPRM0was never touched.GET /x.httpdsrv?target=HTTPDdispatched andrendered, confirming the new guard falls through correctly on success. Member
deleted, server restarted on
HTTPPRM0, production member verified unchanged.Also
make modules/make testclean under-Wall -Werror,make test-host4 suites / 63 assertions / 0 fail.
Not included
The issue's third note — reinstating the malloc-bisecting storage probe now that
malloc()fails with NULL instead of S878 — is marked there as a separateenhancement and is left out.