Found while working on #249, deliberately left alone there to keep that PR to
one subject.
The defect
datasetListHandler() and memberListHandler() both declare:
and then check every write like this:
if ((rc = http_printf(session->httpc, "...")) < 0) goto quit;
rc is unsigned, so rc < 0 is never true. Every one of those checks is dead
code — a failed http_printf() or http_resp() is assigned and discarded,
and the handler keeps writing to a socket that is already gone. A client that
disconnects mid-listing has the whole remaining directory written after it.
It compiles clean because -Wall does not include -Wtype-limits
(that is -Wextra).
Where it is
src/dsapi.c — datasetListHandler()
src/dsapi.c — memberListHandler()
Worth grepping the other handlers in the same pass: the pattern is copied
around, and anywhere rc is unsigned the same checks are inert.
Note on the fix
Changing the declaration to int is a one-word edit but it is not a no-op —
it activates every one of those goto quit paths for the first time. The
cleanup at quit: needs a read in that light before flipping it.
For what it is worth, an early exit there appears harmless today: handle_request()
returns the handler's rc to mvsmf.c, which assigns it to irc and returns 0
regardless, so nothing tries to send a 500 on top of a partly written body. The
headers_sent guard in router.c:157 only covers the abend path.
member_scan() (added in #265) is currently the only place in the member list
where a write failure is actually detected, because it returns int.
Found while working on #249, deliberately left alone there to keep that PR to
one subject.
The defect
datasetListHandler()andmemberListHandler()both declare:and then check every write like this:
rcis unsigned, sorc < 0is never true. Every one of those checks is deadcode — a failed
http_printf()orhttp_resp()is assigned and discarded,and the handler keeps writing to a socket that is already gone. A client that
disconnects mid-listing has the whole remaining directory written after it.
It compiles clean because
-Walldoes not include-Wtype-limits(that is
-Wextra).Where it is
src/dsapi.c—datasetListHandler()src/dsapi.c—memberListHandler()Worth grepping the other handlers in the same pass: the pattern is copied
around, and anywhere
rcisunsignedthe same checks are inert.Note on the fix
Changing the declaration to
intis a one-word edit but it is not a no-op —it activates every one of those
goto quitpaths for the first time. Thecleanup at
quit:needs a read in that light before flipping it.For what it is worth, an early exit there appears harmless today:
handle_request()returns the handler's rc to
mvsmf.c, which assigns it toircand returns 0regardless, so nothing tries to send a 500 on top of a partly written body. The
headers_sentguard inrouter.c:157only covers the abend path.member_scan()(added in #265) is currently the only place in the member listwhere a write failure is actually detected, because it returns
int.