From patchwork Tue May 14 06:11:25 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 1099264 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4536lK75HDz9s00 for ; Tue, 14 May 2019 16:11:41 +1000 (AEST) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="Kq6/Y/R1"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 4536lK5tVCzDqHr for ; Tue, 14 May 2019 16:11:41 +1000 (AEST) X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=axtens.net (client-ip=2607:f8b0:4864:20::633; helo=mail-pl1-x633.google.com; envelope-from=dja@axtens.net; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=axtens.net header.i=@axtens.net header.b="Kq6/Y/R1"; dkim-atps=neutral Received: from mail-pl1-x633.google.com (mail-pl1-x633.google.com [IPv6:2607:f8b0:4864:20::633]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4536lB0F2qzDq9m for ; Tue, 14 May 2019 16:11:33 +1000 (AEST) Received: by mail-pl1-x633.google.com with SMTP id a5so7674486pls.12 for ; Mon, 13 May 2019 23:11:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=H66n6zCicR98m/jgYNLU+Fe+6QGSRMJJ6J/fdFNMuio=; b=Kq6/Y/R1ddAEuLBRdkiAHVlvsmwuAS+N4685a+i9amBxLWLEIWogSFU98ioooTGBUf zmSwYbRZTcsW5kumDSnSDR7aJl6b66bNtWvkfi++SURB6Zi6YpYl7e4WhckfGZGLfuZw PxpcwcQyBdHAj4y5ZkG4YXs2SLQnex/KA0HYo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=H66n6zCicR98m/jgYNLU+Fe+6QGSRMJJ6J/fdFNMuio=; b=qRNATfeHwTqvrvuwUb4741HmsXwP8EYQxoYHm49ZbxKEWxxv3n7076eIjSbhY4lF4C ToGUt5caaYxCWIFnVDQ2lMouexY0hTSvaak1tVar9JMTtfrA2HYm6YdBs7lv2Tg4ehH3 W0jYjaUgcmH7YEGb4sc67t1S85sGdw+DZ28xcZkwOpEUXFxPOqwBV/6WgfLJ/u+jZleC 2CGkivFK4eD3VDNemNr8D6jI7XLN4PY2FRSQirQ4CZ4nfv9F0yC+Npo3AGuOKgBqgJA0 W9lCkeln+ztKabeM3jllu9JfiFWS2vFILdYd3NX4bshl6m4Qyi7ENYjp4bOfs2ZIf6Kg ixuw== X-Gm-Message-State: APjAAAX9cv3PHtTNl8rRcAeq3im2mrRyq93Mx4LQ+J8SC5jSltoARdOE 3w//HK9ZNEvtsH1lkslleInpREP1Fps= X-Google-Smtp-Source: APXvYqxeEouzl3ucyV2EpP70MkirUTdaskrmyt215IA1dSMEfz6nnz8N9Pz2041cZ0lZY6nxRaa0ug== X-Received: by 2002:a17:902:8c81:: with SMTP id t1mr36251237plo.333.1557814289743; Mon, 13 May 2019 23:11:29 -0700 (PDT) Received: from localhost (124-171-207-104.dyn.iinet.net.au. [124.171.207.104]) by smtp.gmail.com with ESMTPSA id r9sm23056628pfc.173.2019.05.13.23.11.28 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 13 May 2019 23:11:28 -0700 (PDT) From: Daniel Axtens To: patchwork@lists.ozlabs.org, jk@ozlabs.org, stephen@that.guru Subject: [PATCH] Revert "parser: Ensure whitespace is stripped for long headers" Date: Tue, 14 May 2019 16:11:25 +1000 Message-Id: <20190514061125.13637-1-dja@axtens.net> X-Mailer: git-send-email 2.19.1 MIME-Version: 1.0 X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" This reverts commit 841f966b8d54b2f51ab1c498eed6e5391f2546a9. In July 2018, we received a report of OzLabs patchwork mangling emails that have subjects containing words with internal commas, like "Insert DT binding for foo,bar" (#197). Stephen took a look and came up with the comment this reverts. Quoting the commit message: RFC2822 states that long headers can be wrapped using CRLF followed by WSP [1]. For example: Subject: Foo bar, baz Should be parsed as: Foo bar,baz As it turns out, this is not the case. Journey with me to section 2.2.3 of RFC 2822: 2.2.3. Long Header Fields Each header field is logically a single line of characters comprising the field name, the colon, and the field body. For convenience however, and to deal with the 998/78 character limitations per line, the field body portion of a header field can be split into a multiple line representation; this is called "folding". The general rule is that wherever this standard allows for folding white space (not simply WSP characters), a CRLF may be inserted before any WSP. For example, the header field: Subject: This is a test can be represented as: Subject: This is a test So the issue with the example in the reverted commit is that there is no folding white space in "bar,baz", so it's not valid to split it. These are valid: Subject: Foo bar,baz Subject: Foo bar,baz but splitting "bar,baz" into "bar,\n baz" is not valid. What then is correct unfolding behaviour? Quoting the RFC again: The process of moving from this folded multiple-line representation of a header field to its single line representation is called "unfolding". Unfolding is accomplished by simply removing any CRLF that is immediately followed by WSP. Each header field should be treated in its unfolded form for further syntactic and semantic evaluation. In other words, the unfolding rule requires you to strip the CRLF, but it does not permit you to strip the WSP. Indeed, if "bar,\n baz" is received, the correct unfolding is "bar, baz". If you do strip the WSP, you end up mashing words together, such as in https://patchwork.ozlabs.org/patch/1097852/ So revert the commit, restoring original behaviour, but keep a corrected version of the test. This presents a big question though: how did Rob's email up with a mangled subject line? To answer this question, you end up having to learn about OzLabs Patchwork and how it differs from Patchwork the project. OzLabs Patchwork (patchwork.ozlabs.org) is an installation of Patchwork. Part of what makes it so useful for so many projects is a little intervening layer that can massage some mail to make it end up in the right project. Email that lands in the device tree project is an example of email that goes through this process. I only learned about this today and I haven't looked in any detail at precisely what is done to the mail. The script is not part of the Patchwork project. This intervening filter is a Python script that runs - and this is an important detail - in Python 2.7. Ignoring all the details, the filter basically operates in a pipe between the mail program and patchwork's parsemail, like (mail from system) | filter.py | parsemail At it's very simplest, filter.py acts as follows: import email import sys mail = email.parse_from_file(sys.stdin) sys.stdout.write(mail.as_string()) Fascinatingly, if you take Rob's email from #197 and put it through this process, you can see that it is getting mangled: Before: Subject: [PATCH v2 3/4] dt-bindings: sound: wm8994: document wlf,csnaddr-pd property After: Subject: [PATCH v2 3/4] dt-bindings: sound: wm8994: document wlf, csnaddr-pd property You can see that python27 has incorrectly wrapped the header, breaking where there is not a foldable space. Python3 does not have this issue. To summarise: - part of the magic of OzLabs PW is a filter to make sure mail gets to the right place. This isn't part of the Patchwork project and so is usually invisible to patchwork developers. - the filter is written in python27. The email module in py27 has a bug that incorrectly breaks subjects around commas within words. - patchwork correctly unfolds those broken subjects with a space after the comma. - thr extra space was interpreted as a bug in patchwork, leading to a misinterpretation of the spec to strip out the whitespace that was believed to be in error. - that broke other wrapped subjects. To solve this, revert the commit and I'll work with jk to get the filter script into py3 compatibility. (Given that py27 sunsets in ~7mo, trying to fix it is not worth it.) Closes: #197 CC: stable --- patchwork/parser.py | 1 - patchwork/tests/test_parser.py | 2 +- releasenotes/notes/issue-197-4f7594db1e4c9887.yaml | 9 +++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/patchwork/parser.py b/patchwork/parser.py index 712780a498c4..91e9920c8782 100644 --- a/patchwork/parser.py +++ b/patchwork/parser.py @@ -47,7 +47,6 @@ class DuplicateMailError(Exception): def normalise_space(value): - value = ''.join(re.split(r'\n\s+', value)) whitespace_re = re.compile(r'\s+') return whitespace_re.sub(' ', value).strip() diff --git a/patchwork/tests/test_parser.py b/patchwork/tests/test_parser.py index ddbcf5b15a19..f18220298078 100644 --- a/patchwork/tests/test_parser.py +++ b/patchwork/tests/test_parser.py @@ -838,7 +838,7 @@ class SubjectTest(TestCase): self.assertEqual(clean_subject("[PATCH] meep \n meep"), ('meep meep', [])) self.assertEqual(clean_subject("[PATCH] meep,\n meep"), - ('meep,meep', [])) + ('meep, meep', [])) self.assertEqual(clean_subject('[PATCH RFC] meep'), ('[RFC] meep', ['RFC'])) self.assertEqual(clean_subject('[PATCH,RFC] meep'), diff --git a/releasenotes/notes/issue-197-4f7594db1e4c9887.yaml b/releasenotes/notes/issue-197-4f7594db1e4c9887.yaml index 2777fbc2f85b..41b86c064b8a 100644 --- a/releasenotes/notes/issue-197-4f7594db1e4c9887.yaml +++ b/releasenotes/notes/issue-197-4f7594db1e4c9887.yaml @@ -1,7 +1,8 @@ --- fixes: - | - Long headers can be wrapped using CRLF followed by WSP (whitespace). This - whitespace was not being stripped, resulting in errant whitespace being - saved for the patch subject. This is resolved though existing patches and - cover letters will need to be updated manually. + Long headers can be wrapped using CRLF followed by WSP (whitespace). There + was an incorrect fix that would lead to whitespace being stripped where it + shouldn't be, resulting in words being stuck together (likethis). This is + resolved, though existing patches and cover letters will need to be + updated manually.