New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cgi.FieldStorage memory usage can spike in line-oriented ops #41503
Comments
Various parts of cgi.FieldStorage call its The simplest fix is to make use of the "size" argument |
Logged In: YES Methinks that the fix isn't quite right: it would Also, there's a call to fp.readline() in parse_multipart() |
Logged In: YES Re: parse_multipart.. yes, it looks like there's no use Good catch wrt boundary recognition bug, I'm uploading |
Logged In: YES Can I tweak you into uploading a unit test? |
Logged In: YES An updated test_cgi.py is attached. I test both the |
Logged In: YES FYI, I'd be happy to do the merging here if you wanted to |
Logged In: YES The files I've just uploaded are revisions to the cgi and test_cgi modules for the FTR, this is a bug which exposes systems which use the cgi.FieldStorage class |
Logged In: YES Doesn't this require a change to test/output/test_cgi or The verify(x == y) should be vereq(x, y). The last chunk of the patch in cgi.py should be: rather than the 4 lines of if/else. I don't know if this patch really addresses the problem or |
Logged In: YES Yup, test/output/test_cgi did need fixing. Apologies, I did not understand As far as verify vs. vereq, the test_cgi module uses verify all over the place. I've uploaded a patch which contains a) the fix to cgi.py, b) the fix to The stylistic change wrt to last_line_lfend is fine with me, but I'll leave that I'm not sure how to ensure the fix doesn't create other problems other than |
Logged In: YES +1. minor nits: in the main patch: instead of + if line.endswith('\n'): you can just use last_line_lfend = line.endswith('\n') in the unit test: instead of if type(a) != type(0): use if not isinstance(a, int): so that if some future release changes file.closed to return Is tehre a reason why you're not patching the fp.readline() |
Logged In: YES BTW it would be better if all patches were in a single file |
Logged In: YES wrt parse_multipart: this function just turns around and puts the output from Returns a dictionary just like parse_qs() keys are the field names, each Is it OK to write a tempfile in this function (e.g. does that make it not useful If not, maybe we should just deprecate parse_multipart? I do find things that I'm uploading another file with your style change suggestions. It bundles all |
Logged In: YES OK, let's forget about parse_multipart(). Feel free to add a |
Logged In: YES Checked in as r51190 (Chris's patch plus a warning added to |
Logged In: YES Is this a backportable fix? |
Logged In: YES Yes, I believe so. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: