incorrect line splitting in HttpRequestParser

[tumb1er]

We are receiving some HTTP requests that cause an Invalid Header error in aiohttp.
That's because of an issue in splitlines in
aiohttp.procotol.HttpRequestParser:

lines = raw_data.decode(
            'ascii', 'surrogateescape').splitlines(True)

For example, this code produces 4 lines instead of 3:

>>> rd = bytearray(b'line1\r\nline2_start_\x1c_line2_end\r\nline3\r\n')
>>> rd.decode('ascii', 'surrogateescape').splitlines(True)
['line1\r\n', 'line2_start_\x1c', '_line2_end\r\n', 'line3\r\n']

In my case it was invalid user-agent header for UCBrowser, but any thoughts how to fix it?

[popravich]

we had the same issue with splitlines when parsing csv.
In our case calling splitlines(True) before decode() solved it. But this means lines must be decoded after splitlines (loop through whole list) or each line must be decoded in place where it is used.

[fafhrd91]

problem with split lines and then decode is parser performance degradation. i tested that in early days of aiohttp development.

@asvetlov @popravich @kxepal ideas?

[popravich]

I've done few simple performance tests and here are the results:

>>> raw = b'\r\n'.join([b'part1\x1c_part2\r\nline2\r\nline3'] * 50000)
>>> raw2 = b'\r\n'.join([b'some-not-very-short-header: and_its_verryyyyyy_looooooooong_value'+b'e'*100] * 10000)
>>> len(raw), len(raw2)
... (1399998, 1669998)

# short lines
>>> %timeit raw.decode('ascii', 'surrogateescape').splitlines(True)
100 loops, best of 3: 14.5 ms per loop
>>> %timeit list(map(lambda b: b.decode('ascii', 'surogateescape'), raw.splitlines(1)))
10 loops, best of 3: 81.2 ms per loop
>>> %timeit next(map(lambda b: b.decode('ascii', 'surogateescape'), raw.splitlines(1)))
100 loops, best of 3: 7.68 ms per loop
>>> %timeit raw.decode('ascii', 'surogateescape').split('\r\n')
100 loops, best of 3: 11.3 ms per loop

# longer lines
>>> %timeit raw2.decode('ascii', 'surrogateescape').splitlines(True)
100 loops, best of 3: 2.68 ms per loop
>>> %timeit list(map(lambda b: b.decode('ascii', 'surogateescape'), raw2.splitlines(1)))
100 loops, best of 3: 7.97 ms per loop
>>> %timeit next(map(lambda b: b.decode('ascii', 'surogateescape'), raw2.splitlines(1)))
100 loops, best of 3: 2.22 ms per loop
>>> %timeit raw2.decode('ascii', 'surogateescape').split('\r\n')
100 loops, best of 3: 3.25 ms per loop

So maybe it makes sense to use next(map... pair?
What your thoughts?

[tumb1er]

next(map(...)) processes only first line, and all other variants process all headers data. Correct test is

def test_next():
    try:
        while True:
            next(map(...))
    except StopIteration:
        pass

[asvetlov]

@fafhrd91
The benchmark from @popravich is incorrect but I like the idea for splitting byte-string and decoding after that.

[popravich]

Sorry for confusion.
Yes, the test isn't correct, morning coffee didn't work)

I meant using iterator that map returns instead of lines list.
I will do some tests on HttpRequestParser with different variants of split lines and come back later.

But I think that splitting bytes and then decoding lines in place where its used might not hit performance too much. Any way I will do some tests.

[tumb1er]

@popravich what about regex splitting? May be it'll both fix split logic and preserve line endings. Without performance penalty.

[fafhrd91]

how often headers encoding is broken?
can we do something like (optimistic approach):

   try:
       lines = raw.decode('ascii').splitlines(True)
   except UnicodeDecodeError:
       lines = split and decode with surrogateescape 

[tumb1er]

It doesn't raise decode error, it just splits extra lines by \x1c symbol. Real exception is in parse_headers:

            try:
                name, value = line.split(':', 1)
            except ValueError:
                raise ValueError('Invalid header: {}'.format(line)) from None

Good idea but hard to implement it.

PS. Exception happens for less than 0.1% of requests for me.

[fafhrd91]

ah! i think this is bug in .splitlines()
.split('\r\n') works right it just removes '\r\n' from strings.

[fafhrd91]

@tumb1er could you tes fix @a6a179c5ad1e011a73610588de3046487244bed1

[fafhrd91]

@asvetlov could you fill python bug report for .splitlines()

[tumb1er]

@fafhrd91, I've tested, it works now.

BTW, \x1c in ASCII is named "File separator, Information separator four" here http://donsnotes.com/tech/charsets/ascii.html
So it may be not a bug in splitlines :)

[fafhrd91]

maybe:)
documentation should say that then.

[asvetlov]

@fafhrd91 I don't understand clean what exactly is wrong with .splitlines() ?

[fafhrd91]

.splitlines() treats this chars \x1c, \x1d,\x1e` as line endings.

[popravich]

well, str.splitlines() treats those chars as line endings and bytes.splitlines() does not. that may a problem.

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.