Possible regression in git-http-backend

I am not sure if this is a regression or not, but I wanted to get feedback.

It looks like this commit changed some behavior in git-http-backend

https://git.kernel.org/pub/scm/git/git.git/commit/?id=6bc0cb5176a5e42ca4a74e3558e8f0790ed09bb1

The change that it has made is that it no git-upload-pack hangs when uwsgi doesn't close stdin.

http://lists.unbit.it/pipermail/uwsgi/2016-April/008440.html

This was the only place I could find this discussed online for uwsgi (but I did find several blog posts comments mentioning the issue). It looks like it was worked around in uwsgi >= 2.0.13 by adding the setting `cgi-close-stdin-on-eof = true`. But there was some discussion last year that this shouldn't be needed based on the RFC for cgi?

http://lists.unbit.it/pipermail/uwsgi/2016-May/008462.html http://web.archive.org/web/20100212094332/http://hoohoo.ncsa.illinois.edu/cgi/in.html

""" The server will send CONTENT_LENGTH bytes on this file descriptor. Remember that it will give the CONTENT_TYPE of the data as well. The server is in no way obligated to send end-of-file after the script reads CONTENT_LENGTH bytes. """

Here is a gist for providing a config setup, I used the latest nginx from the nginx repo for centos 7 and uwsgi 2.0.14 from epel. The setup requires starting nginx.service and uwsgi.service and creating a bare repo in /srv/git/ and running

https://gist.github.com/gtmanfred/8b2e26b07db3c75094d0607048b2c828

Thanks, Daniel

On Tue, Apr 25, 2017 at 05:39:16PM -0600, Daniel Wallace wrote:

Yeah, I think that could be considered an unintended regression. The original code _also_ didn't respect CONTENT_LENGTH and would potentially just keep reading. But because it was reading formatted data, as long as the data was well-formed, it would generally stop reading where the webserver expected it. So it mostly worked; even though there were cases that would hang, they were presumably rare.

People using IIS ran into a similar thing, I think. There was a patch, but it had a lot of problems. I do think it would be reasonable to:

  1. Respect CONTENT_LENGTH if it's set, and never read more than that
     many bytes.

  2. Handle sentinel values for CONTENT_LENGTH that tell us the data is
     streaming in and we must read until EOF (Apache leaves
     CONTENT_LENGTH unset in this case, and apparently IIS sets it to
     -1).

  3. Ideally, do both of those not just in the buffering case added by
     6bc0cb517, but for other input as well. That might be hard, though,
     because I think we literally hand off the descriptor to
     sub-programs for some operations. So even if we just handled the
     one case, that would at least fix any regressions caused by
     6bc0cb517.

Here are links to the previous discussion:

  http://public-inbox.org/git/F0F5A56A22F20D4CB4A03BB8D6658797E260E0E3@SERVER2011.CS-SOFTWARE.local/

  http://public-inbox.org/git/F0F5A56A22F20D4CB4A03BB8D6658797E261A3D6@SERVER2011.CS-SOFTWARE.local/

-Peff