| Message ID | 4364b38bd027c219d41282bad3f8476daec936f9.1590154401.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-curl: fix deadlocks when remote server disconnects | expand |
On Fri, May 22, 2020 at 09:33:47AM -0400, Denton Liu wrote: > In the CGI scripts which emulate a connection being prematurely > terminated, it doesn't make sense for there to be a trailing newline > after the simulated connection cut. Remove these newlines. Ah, good catch. I think in the first one it doesn't matter: > -printf "%s%s\n" "0079" "45" > +printf "%s%s" "0079" "45" because we have a too-short packet, so the fact that it is 3 bytes and not 2 does not change that. I agree it's clearer without the newline, though. I wonder if: printf "0079" "fewer than 0x79 bytes" would make it even more self-documenting. :) > -printf "%s\n" "00" > +printf "%s" "00" This one is a behavior improvement: we were probably hitting "oops, newline isn't a valid line-length character" before, and now we're really hitting the truncated packet. I don't know if it's worth adding an extra test with a bogus line-length. I'm OK with with it either way. -Peff
Hi Peff, On Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote: > > -printf "%s\n" "00" > > +printf "%s" "00" > > This one is a behavior improvement: we were probably hitting "oops, > newline isn't a valid line-length character" before, and now we're > really hitting the truncated packet. The test currently actually greps for the "incomplete length" error message and it passes so the behaviour remains the same. We just got lucky that we send "00" instead of "000" beacuse "000\n" would've otherwise given us a full length header. > I don't know if it's worth adding an extra test with a bogus > line-length. I'm OK with with it either way. I think I'll leave this unless anyone really wants this to be tested. Thanks, Denton
On Fri, May 22, 2020 at 12:05:52PM -0400, Denton Liu wrote: > Hi Peff, > > On Fri, May 22, 2020 at 11:54:10AM -0400, Jeff King wrote: > > > -printf "%s\n" "00" > > > +printf "%s" "00" > > > > This one is a behavior improvement: we were probably hitting "oops, > > newline isn't a valid line-length character" before, and now we're > > really hitting the truncated packet. > > The test currently actually greps for the "incomplete length" error > message and it passes so the behaviour remains the same. We just got > lucky that we send "00" instead of "000" beacuse "000\n" would've > otherwise given us a full length header. Ah, OK. I was surprised it didn't hit your new code in check_pktline() that complains about non-hex chars. But the reason is that we don't check digit-by-digit as we read them. We wait until we have 4 chars, and then we feed the whole thing to packet_length(). But since we only receive 3, the state machine doesn't move to that step. So that makes sense. > > I don't know if it's worth adding an extra test with a bogus > > line-length. I'm OK with with it either way. > > I think I'll leave this unless anyone really wants this to be tested. Sounds good. -Peff
diff --git a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh index 2f5ed9fcf6..90e73ef8d5 100644 --- a/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh +++ b/t/lib-httpd/incomplete-body-upload-pack-v2-http.sh @@ -1,3 +1,3 @@ printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result" echo -printf "%s%s\n" "0079" "45" +printf "%s%s" "0079" "45" diff --git a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh index 86c6e648c9..dce552e348 100644 --- a/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh +++ b/t/lib-httpd/incomplete-length-upload-pack-v2-http.sh @@ -1,3 +1,3 @@ printf "Content-Type: text/%s\n" "application/x-git-upload-pack-result" echo -printf "%s\n" "00" +printf "%s" "00"
In the CGI scripts which emulate a connection being prematurely terminated, it doesn't make sense for there to be a trailing newline after the simulated connection cut. Remove these newlines. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/lib-httpd/incomplete-body-upload-pack-v2-http.sh | 2 +- t/lib-httpd/incomplete-length-upload-pack-v2-http.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)