Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

Subject: Re: [PATCH] emacs: notmuch-show: remove extraneous shell quoting

Date: Fri, 16 Sep 2016 20:36:27 +0100

To: Matt Armstrong, notmuch@notmuchmail.org, Tomi Ollila

Cc:

From: Mark Walters


Hi

On Fri, 16 Sep 2016, Matt Armstrong <marmstrong@google.com> wrote:
> Tomi, thanks for your reply asking for some motivation behind this
> patch.  I can't reply directly to your message because, for some reason,
> it doesn't appear in my mailbox (I discovered your message while reading
> the mail archive on notmuchmail.org).
>
> The code dealing with this quoting issue was last touched in commit
> b57d9635f50d5e9b53092218e81f6d2c391c363e, where Carl recognizes the
> quoting is a bit of a hack and asks for a better fix.  This is my
> attempt.
>
> I am motivated by a concern for code health.  I saw the quoting, did not
> understand it, recognized it as probably wrong, investigated how the
> quotes were actually passed from Emacs to the shell, and still believed
> it wrong.
>
> I think this kind of flaw can be placed in the category of security fix.
> Quoting issues often are.  But, I'm not a security person.

I think all the data being passed is generated by notmuch so I don't
see a security issue.

> By my reasoning, the rationale for the change is simple:
>
> a) It is the job of notmuch elisp to pass call-process the args in an
> appropriate manner for notmuch-command (which is always a local
> command).  Because call-process takes a list of strings, and no shell is
> involved, using shell quotes is wrong.  It just so happens that Xapian
> ignores the quotes, but taking advantage of that is not a great thing.
>
> b) If notmuch-command is doing something fancy, as is the case with the
> "remote" script on https://notmuchmail.org/remoteusage/, it is the job
> of that script to quote its own args properly for ssh.  It looks like it
> already does this.

That one script does -- there are at least two others even on the wiki
(see the links at the bottom of the above page) -- they also seem to be
fine. But there could be other user scripts that do need the quoting.

So the question is do we mind breaking a few currently working setups
for the purpose of a mild cleanup. Since the current code is confusing I
think a comment would be in order if we don't apply this patch.

Best wishes

Mark


> So, the quoting is unnecessary on both accounts.
>
>
> Matt Armstrong <marmstrong@google.com> writes:
>
>> Remove shell quoting from notmuch-show--build-buffer.  The args list
>> is ultimately passed to call-process, which passes them verbatim to
>> the subprocess (typically, notmuch).  The quoting, intended for a
>> shell, is unnecessary and confusing.
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Thread: