Handle long lines #59

Mortal · 2017-03-29T03:11:22Z

I've found two test cases that cause the current aiosmtpd to fail with an unhandled exception. The issue is that StreamReader.readline raises a ValueError when its buffer overflows. The issue can arise both when reading a command line and reading a text line in DATA.

This PR sets a lower limit on StreamReader (1000 bytes per RFC 5321 4.5.3.1.6. + 1 byte for a leading transparent dot) to make the buffer overflow more frequent, and then it adds more complicated handling of StreamReader.readuntil in command line reading and DATA line reading. I couldn't get it working with StreamReader.readline, but since its implementation simply calls readuntil and masks some exceptions, it seems like we should use readuntil anyway.

I'm not sure exactly what to do in case of an early EOF -- should the code simply send some low-level SMTP error back to the client and return? Right now my implementation is (too) lenient about EOFs (materializing as asyncio.IncompleteReadError exceptions).

Mortal · 2017-04-04T03:21:52Z

After digging into the circumstances surrounding early EOF or connection reset from client I have found that cancelling the _handle_client coroutine on connection reset is the sanest thing to do, and then simply catching CancelledError at the appropriate points in _handle_client.

Currently the Python 3.4 build fails with stalling on Travis in controller.rst. I don't know why that is.

I've managed to add tests that keep coverage at 100% which was not an easy task. However, most of the tasks merely consist of opening an SMTP session and closing/resetting at odd times without asserting that the output of aiosmtpd is anything useful -- it only ensures that no exception is raised.

Mortal · 2017-04-19T18:15:08Z

This PR conflicts with #70 as they get rid of self._connection_closed in two different ways.

kozzztik · 2017-04-20T15:18:09Z

I dont see conflict here. Your PR completely remove self._connection_closed usage, but #70 replace it with self.transport. You can still not use it, even it is replaced )

Lift some additional (non-line-length) tests from #59.

* Test cases for connection problems during readline(). Also, add some much improved debugging options for when the test suite is run with the -E option. Update the documentation. * Some lines won't be covered in Python 3.4 * Refactor test class. Lift some additional (non-line-length) tests from #59. * Remove redundant test cases. Closes #62

warsaw · 2017-05-08T00:04:25Z

In the interest of getting 1.0 before Pycon, and since this branch has conflicts, I'm going to defer it past 1.0

Mortal · 2017-06-24T19:22:19Z

Rebased on top of #115.

waynew · 2018-06-16T12:50:15Z

@Mortal do you have any interest in continuing this PR? There's apparently a conflict that needs to be resolved.

Mortal · 2018-06-17T10:26:59Z

@waynew I have rebased onto master and squashed all commits into a single commit with a proper commit message.

waynew

I think I'd like to see this setting made configurable, It would also be great to have some documentation that cites https://www.ietf.org/rfc/rfc2821.txt

aiosmtpd/smtp.py

Mortal · 2018-06-24T14:19:41Z

I'll try to have a look at making the limit configurable on Tuesday.

waynew · 2019-06-01T14:47:44Z

@Mortal any update on this PR, too?

Mortal · 2019-06-02T13:45:56Z

As with the other PR, [help-wanted] is probably the best way forward.

pepoluan · 2020-10-25T16:06:04Z

I'm still thinking about the necessity of limiting StreamReader to N bytes (1001 in the PR).

Can we be sure that the underlying TCP stack will not buffer things until 1 page (4096 octets) and deliver things in bulk?

Yes, I know of the PSH flag, but IIRC it was never a requirement ("MUST"), just a recommendation ("SHOULD") for TCP stack implementation to actually deliver the data to the app when PSH flag is received.

CLAassistant · 2020-11-16T10:22:25Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Mortal · 2020-11-17T10:16:41Z

Hi @pepoluan, thanks for picking up this project! I don't really have the headspace these days to keep working on aiosmtpd, sorry. Can you confirm that the notification from CLAassistant is legit? I tried googling and I couldn't find any news anywhere that aiosmtpd specifically or aio-libs generally have started using CLAs. If you can confirm then I'll gladly sign a CLA.

pepoluan · 2020-11-20T13:41:29Z

Hi @Mortal , sorry for not answering sooner. This week had been a bit hectic in my life...

The CLA is legit, set by the maintainers of the aio-libs project, however there seems to be some... opinion about it. It's being discussed in #208 .

I myself am personally neutral about it, and I also personally think there's nothing wrong with the words, but it's still an interesting discussion over there (with links to other comments in other PRs).

I won't stop you from signing the CLA, but at the moment I also am not requiring anyone to sign the CLA. Just do what you think is right 😉

(For the record, I have already signed it.)

So the response sender will send a response based on first non-nominal event. A test case has been added, and a previous test case had comments added as well to explain what's happening. (Also changed that other test case's name to more properly reflect what's being tested)

These are all faster, tested with lots of timeit's. In relation to aio-libs#149 ... well, there's no relation, actually. `data` is an internal temporary structure and after removal of "transparent dots" gets flattened into bytes anyways. The visible external structs are `original_content` and `content`, and their data types remain the same.

pepoluan

Some comments on some new stuff

pepoluan · 2020-12-02T07:23:26Z

aiosmtpd/smtp.py

            if text.startswith(b'.'):
-                data[i] = text[1:]
-        content = original_content = EMPTYBYTES.join(data)
+                del text[0]


timeit shows that doing it this way is 10%-20% faster. Not only that no copy is being done, also we can do away with counter increment of enumerate.

pepoluan · 2020-12-02T09:08:11Z

Okay, I've tested on my systems (Windows, WSL, FreeBSD, and various Ubuntu), and they all pass with flying colors.

So I'm un-draft-ing this for review & merge.

aiosmtpd/smtp.py

Previous behavior _seems to be_ non-compliant with SMTP Server specification. Although the rebuilt smtp_DATA collection loop has fixed this non-compliance, we want to make sure that future changes will not regress to non-compliance. Also one test case has been moved to make the sequence more logical.

Somehow PyCharm's code inspection insists that self.handler.session is of type Session defined somewhere else and not within the aiosmtpd.smtp module.

Mostly to silence PyCharm's code inspection that complains of patch not fulfilling enter_context's signature. But the side effect is cleaner, simpler code.

This time, affects test_smtp.py

Also its test case

@patch

Because patching a variable does NOT return a value, so those @patch lines do NOT need their respective args. (Also, correct the ordering on args on some of the test cases)

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

Seems to be tox+unittest behavior to pass-through WARNING-level notifications (results of `warnings.warn` or `Logger.warning`) These warnings are harmless, but messes up tox+unittest output, so we suppress them.

"alpha" should indicate only "stages" of implementation as we get nearer to release. NEWS.rst should instead show/summarize _all_ the new (and good) things in release. Or, in other words, a sum total of all the "alpha"s and "beta"s and "rc"s prior to release. "aiosmtpd-next" stays because it's the 'date' of release. Kind of like "HEAD" in git.

This second change is actually even more important, standards-compliance-wise.

pepoluan

Some comments on the latest changes.

pepoluan · 2020-12-07T06:35:43Z

aiosmtpd/smtp.py

            if text.startswith(b'.'):
-                data[i] = text[1:]
-        content = original_content = EMPTYBYTES.join(data)


If content is going to be overwritten anyways, why set its value here.

Moved actual setting-of-value inside the if branches.

pepoluan · 2020-12-07T06:36:56Z

aiosmtpd/testing/helpers.py

+            result.append(sock.recv(1024))
+        else:
+            break
+    return b"".join(result)


I need this func to perform low-level non-hanging send-and-receive for each message line during DATA phase.

pepoluan · 2020-12-07T06:47:21Z

Testing progress on my systems:

Windows 10 (via PyCharm runner): (ALL)
Ubuntu 18.04 on WSL: (ALL) + pypy3-{nocov,cov}
FreeBSD 12.1 on VBOx: (ALL) + pypy3-nocov
Windows 10 (via PSCore 7.0.3): (ALL)
Windows 10 (Cygwin): qa,py36-{nocov,cov}

I think I'll skip testing on my VBox Ubuntu 18.04+20.04 combo. There should be no difference compared to the GitHub Actions ones.

pepoluan · 2020-12-08T03:39:57Z

With your blessings @waynew , we can get this merged :-)

pepoluan · 2020-12-12T04:45:01Z

Come Monday I'll merge this if no objection.

Protocol violation (see #9 ) is a Serious Bug™ and should be fixed asap.

Mortal mentioned this pull request Mar 29, 2017

Add test for changing SMTP.command_size_limit #56

Closed

Mortal force-pushed the handle_long_lines branch 3 times, most recently from efd0709 to 65f1429 Compare April 4, 2017 02:12

Mortal force-pushed the handle_long_lines branch from 65f1429 to f7d0cf0 Compare April 4, 2017 03:22

warsaw mentioned this pull request Apr 26, 2017

stream reader read line must be in try catch because of a lost connection #61

Closed

warsaw added a commit that referenced this pull request Apr 26, 2017

Refactor test class.

388cb94

Lift some additional (non-line-length) tests from #59.

warsaw mentioned this pull request Apr 26, 2017

Handle lost connections and EOF during readlines #73

Merged

warsaw added this to the post-1.0 milestone May 8, 2017

Mortal mentioned this pull request Jun 9, 2017

CancelledError and misbehaving clients handling #110

Closed

Mortal force-pushed the handle_long_lines branch from f7d0cf0 to de3ad56 Compare June 24, 2017 19:19

duck-rh mentioned this pull request Sep 11, 2017

Problem handling incoming LMTP mails mailman/mailman#152

Closed

Mortal force-pushed the handle_long_lines branch from 9ae0353 to 702b2dd Compare June 17, 2018 10:10

waynew approved these changes Jun 17, 2018

View reviewed changes

waynew requested changes Jun 23, 2018

View reviewed changes

aiosmtpd/smtp.py Outdated Show resolved Hide resolved

pepoluan mentioned this pull request Dec 2, 2020

Protocol violation and message receive when exceeding data limit size #9

Closed

pepoluan linked an issue Dec 2, 2020 that may be closed by this pull request

Protocol violation and message receive when exceeding data limit size #9

Closed

pepoluan added blocker bug labels Dec 2, 2020

pepoluan added 3 commits December 2, 2020 11:42

Correct a wrong comment

cc2ba99

pepoluan force-pushed the handle_long_lines branch from 2dcefcd to a3c9543 Compare December 2, 2020 07:20

pepoluan reviewed Dec 2, 2020

View reviewed changes

pepoluan marked this pull request as ready for review December 2, 2020 09:08

pepoluan requested a review from waynew December 2, 2020 09:15

Mortal commented Dec 3, 2020

View reviewed changes

aiosmtpd/smtp.py Outdated Show resolved Hide resolved

pepoluan added 12 commits December 4, 2020 12:26

Silence code inspection warnings

b4dcbd4

Somehow PyCharm's code inspection insists that self.handler.session is of type Session defined somewhere else and not within the aiosmtpd.smtp module.

Convert some ExitStack to @patch

5684a75

Mostly to silence PyCharm's code inspection that complains of patch not fulfilling enter_context's signature. But the side effect is cleaner, simpler code.

More ExitStack-to-@patch conversion

35df5d6

This time, affects test_smtp.py

Fix housekeep.py spacing

0d3b8c4

Implement defensive line-length check

3440476

Also its test case

Fix off by one args

ab25bc2

Because patching a variable does NOT return a value, so those @patch lines do NOT need their respective args. (Also, correct the ordering on args on some of the test cases)

Merge branch 'master' into handle_long_lines

0ff4a52

# Conflicts: # aiosmtpd/docs/NEWS.rst # aiosmtpd/smtp.py # aiosmtpd/tests/test_smtp.py

Suppress spurious (but harmless) WARNINGs

e4eee55

Seems to be tox+unittest behavior to pass-through WARNING-level notifications (results of `warnings.warn` or `Logger.warning`) These warnings are harmless, but messes up tox+unittest output, so we suppress them.

Rewrite SIZE constant to a nice power-of-two repr

2be15c8

Update NEWS.rst with another important change

0e5f042

This second change is actually even more important, standards-compliance-wise.

pepoluan reviewed Dec 7, 2020

View reviewed changes

pepoluan merged commit 73b7284 into aio-libs:master Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle long lines #59

Handle long lines #59

Mortal commented Mar 29, 2017 •

edited

Loading

Mortal commented Apr 4, 2017

Mortal commented Apr 19, 2017

kozzztik commented Apr 20, 2017 •

edited

Loading

warsaw commented May 8, 2017

Mortal commented Jun 24, 2017

waynew commented Jun 16, 2018

Mortal commented Jun 17, 2018

waynew left a comment

Mortal commented Jun 24, 2018

waynew commented Jun 1, 2019

Mortal commented Jun 2, 2019

pepoluan commented Oct 25, 2020

CLAassistant commented Nov 16, 2020

Mortal commented Nov 17, 2020

pepoluan commented Nov 20, 2020

pepoluan left a comment

pepoluan Dec 2, 2020

pepoluan commented Dec 2, 2020 •

edited

Loading

pepoluan left a comment

pepoluan Dec 7, 2020

pepoluan Dec 7, 2020

pepoluan commented Dec 7, 2020 •

edited

Loading

pepoluan commented Dec 8, 2020

pepoluan commented Dec 12, 2020

Handle long lines #59

Handle long lines #59

Conversation

Mortal commented Mar 29, 2017 • edited Loading

Mortal commented Apr 4, 2017

Mortal commented Apr 19, 2017

kozzztik commented Apr 20, 2017 • edited Loading

warsaw commented May 8, 2017

Mortal commented Jun 24, 2017

waynew commented Jun 16, 2018

Mortal commented Jun 17, 2018

waynew left a comment

Choose a reason for hiding this comment

Mortal commented Jun 24, 2018

waynew commented Jun 1, 2019

Mortal commented Jun 2, 2019

pepoluan commented Oct 25, 2020

CLAassistant commented Nov 16, 2020

Mortal commented Nov 17, 2020

pepoluan commented Nov 20, 2020

pepoluan left a comment

Choose a reason for hiding this comment

pepoluan Dec 2, 2020

Choose a reason for hiding this comment

pepoluan commented Dec 2, 2020 • edited Loading

pepoluan left a comment

Choose a reason for hiding this comment

pepoluan Dec 7, 2020

Choose a reason for hiding this comment

pepoluan Dec 7, 2020

Choose a reason for hiding this comment

pepoluan commented Dec 7, 2020 • edited Loading

pepoluan commented Dec 8, 2020

pepoluan commented Dec 12, 2020

Mortal commented Mar 29, 2017 •

edited

Loading

kozzztik commented Apr 20, 2017 •

edited

Loading

pepoluan commented Dec 2, 2020 •

edited

Loading

pepoluan commented Dec 7, 2020 •

edited

Loading