Opened 7 years ago

Closed 6 years ago

#4317 defect closed fixed (fixed)

twisted.web.http.Request allows you to write to a finished request.

Reported by: David Reid Owned by:
Priority: normal Milestone:
Component: web Keywords: easy
Cc: Branch: branches/request-write-after-finish-4317
branch-diff, diff-cov, branch-cov, buildbot
Author: hderms

Description

This can cause corruption to subsequent requests, it is also partially responsible for the behavior described in #3684

Attachments (3)

ticket 4317 patch.patch (380 bytes) - added by hderms 6 years ago.
Simple fix, uses pythonic idiom that was used in the same class
ticket 4317 patch v2.patch (1.9 KB) - added by hderms 6 years ago.
Rewrite of patch and addition of unit test
ticket 4317 patch v3.patch (1.6 KB) - added by hderms 6 years ago.
adjusted changes requested by reviewer

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 years ago by Jean-Paul Calderone

Keywords: easy added

We should change Request.write to discard such bytes. We should also introduce a warning for this case (that could be done separately though). And ultimately, this should result in an exception.

Changed 6 years ago by hderms

Attachment: ticket 4317 patch.patch added

Simple fix, uses pythonic idiom that was used in the same class

comment:2 Changed 6 years ago by hderms

Keywords: review added
Owner: jknight deleted

comment:3 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to hderms

Thanks. We need a unit test which verifies that this behaves as desired, too. Can you add that to the patch?

comment:4 Changed 6 years ago by hderms

I will do so. I was confused and thought unit-tests were only required when adding new features. Will read the twisted documentation on unit tests and then implement.

Changed 6 years ago by hderms

Attachment: ticket 4317 patch v2.patch added

Rewrite of patch and addition of unit test

comment:5 Changed 6 years ago by hderms

Keywords: review added
Owner: hderms deleted

Added a new patch which raises a RuntimeError when Request.write is called after Request.finish. Added a unit test which tests this functionality.

comment:6 Changed 6 years ago by therve

Author: hderms
Keywords: review removed
Owner: set to hderms
  • + if self.finished == 1: You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...
  • twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable fails after your fix is applied. I suspect the test is broken, but it should be fixed.
  • -        self.assertRaises(RuntimeError, req.finish)
    +        self.assertRaises(RuntimeError, req.finish())
    
    This change is broken, it makes the test fails.
  • There should be 2 blank lines before your test
  • The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?

Thanks!

comment:7 in reply to:  6 Changed 6 years ago by hderms

Replying to therve:

  • + if self.finished == 1: You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...
  • twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable fails after your fix is applied. I suspect the test is broken, but it should be fixed.
  • -        self.assertRaises(RuntimeError, req.finish)
    +        self.assertRaises(RuntimeError, req.finish())
    
    This change is broken, it makes the test fails.
  • There should be 2 blank lines before your test
  • The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?

Thanks!

I examined that test that fails, and I can't seem to find any connection between it and /web/http, it doesn't import anything http related, and the test in particular that fails is just parsing strings. I'm not entirely sure but I think the test is broken as well.

I didn't implement the if self.finished == 1: and I found that to be rather unpythonic as well, but was reluctant to change it because this is my first patch.

The change to the the test_finishAfterConnectionLost(self): was unintentional. I think it resulted from me examining how these unit tests worked, I must have forgotten to eliminate that portion. I will remove it and resubmit.

I will fix the formatting errors.

comment:8 Changed 6 years ago by therve

Hum sorry, the test I mentioned (twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable) was probably just a output parsing error on my part. It must have been the other one.

Changed 6 years ago by hderms

Attachment: ticket 4317 patch v3.patch added

adjusted changes requested by reviewer

comment:9 Changed 6 years ago by hderms

Keywords: review added
Owner: hderms deleted

comment:10 Changed 6 years ago by Jean-Paul Calderone

Author: hdermsexarkun, hderms
Branch: branches/request-write-after-finish-4317

(In [30650]) Branching to 'request-write-after-finish-4317'

comment:11 Changed 6 years ago by Jean-Paul Calderone

(In [30651]) Apply patch from the ticket, with a couple trivial changes

refs #4317

comment:12 Changed 6 years ago by Jean-Paul Calderone

(In [30652]) news fragment

refs #4317

comment:13 Changed 6 years ago by Jean-Paul Calderone

Author: exarkun, hdermshderms
Keywords: review removed
Owner: set to Jean-Paul Calderone
Status: newassigned

Did you manually edit the patch to remove the last hunk? However the latest version of the patch was created, neither patch nor emacs is happy with it (Emacs says "Hunk seriously messed up"). It's not a problem because this patch is short enough that I can manually apply it. Whatever happened though, try to avoid it in the future. :)

I checked the changes into a branch, added a news fragment, and made the if self.finished: change therve suggested. Build results look good, so I'll go ahead and merge this. Thanks!

comment:14 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(In [30653]) Merge request-write-after-finish-4317

Author: hderms Reviewer: therve, exarkun Fixes: #4317

In Request.write, check to see if the response has already been finished and raise an exception if it has, rather than tossing garbage bytes onto the connection.

comment:15 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.