From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#63941: [PATCH] ; always CRLF before non-first boundary in multipart form Date: Tue, 18 Jul 2023 15:04:45 -0400 Message-ID: References: <837csf4fp8.fsf@gnu.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39104"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 63941@debbugs.gnu.org, Eli Zaretskii To: ozzloy Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Jul 18 21:05:20 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qLq0d-0009vA-Hh for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 18 Jul 2023 21:05:19 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qLq0Q-0007Nm-IV; Tue, 18 Jul 2023 15:05:06 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qLq0M-0007Mb-Hz for bug-gnu-emacs@gnu.org; Tue, 18 Jul 2023 15:05:03 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qLq0M-00067d-8z for bug-gnu-emacs@gnu.org; Tue, 18 Jul 2023 15:05:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qLq0L-000885-SM for bug-gnu-emacs@gnu.org; Tue, 18 Jul 2023 15:05:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 18 Jul 2023 19:05:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 63941 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 63941-submit@debbugs.gnu.org id=B63941.168970709631235 (code B ref 63941); Tue, 18 Jul 2023 19:05:01 +0000 Original-Received: (at 63941) by debbugs.gnu.org; 18 Jul 2023 19:04:56 +0000 Original-Received: from localhost ([127.0.0.1]:54311 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qLq0G-00087j-A9 for submit@debbugs.gnu.org; Tue, 18 Jul 2023 15:04:56 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:27875) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qLq0D-00087U-Kw for 63941@debbugs.gnu.org; Tue, 18 Jul 2023 15:04:54 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 28CD1442B63; Tue, 18 Jul 2023 15:04:48 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 60FFC442B5E; Tue, 18 Jul 2023 15:04:46 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1689707086; bh=l8oLHd+zIKEwFQNPGKVDWB/AhgOUDuYlJ+s1P5RkO7M=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=ZIzVqnHGKioQEyPq9c6HzOZNkXXtm/i8t39q8L8paGXIW/9dHgJAjbWBm210qB88x StE3vfDk3LH4EV+MeF0/WHI9sG3wnrT9TB5jwqZ5mnQZF4kUelDhLy6KbZn31d45UG KN7gK70He1zZe63itQdih7oxT4tOYZ0b+yHJs0y/8gHaIXQZz5AEwkJku0uDRz7ad2 mgF7HWd6xr9hHxvpHHLJ3URGExbW/rhHVxrfYv0jJceYpO3NPsTgkyp8IlsRT53JSf 7T266TGxvMzKHJo6E2rT5XNW5lkbP0zT7/VClMSVvOQ7aMmOPra6QMnuYKflyzIR5p Kx2BX0z/kaI2Q== Original-Received: from alfajor (unknown [23.233.149.155]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 2B0761201E4; Tue, 18 Jul 2023 15:04:46 -0400 (EDT) In-Reply-To: (ozzloy@gmail.com's message of "Wed, 7 Jun 2023 19:48:29 -0700") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:265494 Archived-At: OK, I'm not super familiar with this code, and while I really appreciate your tests, I'd rather not completely replace the old with a brand new code because that makes it hard for me to see what's really changed. So, as a first step I took the existing code, and did the following: - "Inline" the (unless (bolp) (insert "\r\n")) into every branch of the preceding `cond`. We know this makes no difference. - Remove the (unless (bolp) (insert "\r\n")) in the "submit" branch since it follows an insertion that ends in "n" and hence doesn't do anything. - Remove the `(unless (bolp)` test from the "file" branch since that's what this bug report is about: - when `filedata` is a number, this makes no difference (we just inserted that number without a trailing \n). - when `filedata` is a string that ends in non-\n (a case that currently works right) this makes no difference. - when it does end in \n this does make a difference which fixes this bug. - when `filedata` is an empty string, this add an additional \r\n compared to the current code. This seems right to me (I expect the decoding software will skip the \r\n at the of the header and then look for \r\n, so it's important to have two \r\n). - In the default case, I left the code as is, but the `(unless (bolp)` test is probably not right. There remain some questions on this patch: 1- When `filedata` is neither a number nor a string this is treated the same as an empty string. Is that right? Or should it just never happen (in which case we should probably catch this case and signal an error). 2- Should we also remove the `(unless (bolp)` in the default case? I think we do (and my reading of your tests agrees, tho I couldn't run your test suite on this version of the code, among other things because it contains multiple tests with the same name, so it gives me things like (error "Test `test-mm-url-encode-multipart-form-data-A-ab-c' redefined")). I suspect we can also simply this code by moving the first (insert "--" boundary "\r\n") before the loop, and the second into the loop so we can make it insert "\r\n--" boundary "\r\n" (and thus remove \r\n from the end of each of the preceding branches). Stefan diff --git a/lisp/gnus/mm-url.el b/lisp/gnus/mm-url.el index 11847a79f17..3041ad16264 100644 --- a/lisp/gnus/mm-url.el +++ b/lisp/gnus/mm-url.el @@ -430,16 +430,17 @@ mm-url-encode-multipart-form-data (insert filedata)) ;; How can this possibly be useful? ((integerp filedata) - (insert (number-to-string filedata)))))) + (insert (number-to-string filedata))))) + (insert "\r\n")) ((equal name "submit") (insert "Content-Disposition: form-data; name=\"submit\"\r\n\r\nSubmit\r\n")) (t (insert (format "Content-Disposition: form-data; name=%S\r\n\r\n" name)) - (insert value))) - (unless (bolp) - (insert "\r\n")))) + (insert value) + (unless (bolp) + (insert "\r\n")))))) (insert "--" boundary "--\r\n") (buffer-string)))