GNU bug report logs - #33344
26.1; doc-view bounding-box recognition doesn't work on path names with spaces

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33344 in the body.
You can then email your comments to 33344 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Spillner <trent2 <at> web.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Sun, 11 Nov 2018 12:36:04 +0100

Hi,

in doc-view.el, line 1215, ghostscript is called via shell to grep the
bounding box of a ps or pdf-file. In line 1218, the last %s
(representing the filename) is used unquoted. Therefore, whenever the
filename or the directory pointing to this file has spaces in its name
gs won't be able to find it and determine a bounding box.

Please change line 1218 from

  (format "-dFirstPage=%s -dLastPage=%s %s"

to

  (format "-dFirstPage=%s -dLastPage=%s \"%s\""

Thank you!

Message #8 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Robert Spillner <trent2 <at> web.de>
Cc: 33344 <at> debbugs.gnu.org
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Mon, 12 Nov 2018 16:36:48 -0500

Robert Spillner wrote:

> in doc-view.el, line 1215, ghostscript is called via shell to grep the
> bounding box of a ps or pdf-file. In line 1218, the last %s
> (representing the filename) is used unquoted. Therefore, whenever the
> filename or the directory pointing to this file has spaces in its name
> gs won't be able to find it and determine a bounding box.
>
> Please change line 1218 from
>
>   (format "-dFirstPage=%s -dLastPage=%s %s"
>
> to
>
>   (format "-dFirstPage=%s -dLastPage=%s \"%s\""

Thanks for the report.
I don't see a need for the shell to be involved at all here.
The following seems to work for me - how about for you?


--- i/lisp/doc-view.el
+++ w/lisp/doc-view.el
@@ -1204,25 +1204,30 @@ doc-view-set-slice-using-mouse
 
 (defun doc-view-get-bounding-box ()
   "Get the BoundingBox information of the current page."
-  (let* ((page (doc-view-current-page))
-	 (doc (let ((cache-doc (doc-view-current-cache-doc-pdf)))
-		(if (file-exists-p cache-doc)
-		    cache-doc
-		  doc-view--buffer-file-name)))
-	 (o (shell-command-to-string
-	     (concat doc-view-ghostscript-program
-		     " -dSAFER -dBATCH -dNOPAUSE -q -sDEVICE=bbox "
-		     (format "-dFirstPage=%s -dLastPage=%s %s"
-			     page page doc)))))
-    (save-match-data
-      (when (string-match (concat "%%BoundingBox: "
-				  "\\([[:digit:]]+\\) \\([[:digit:]]+\\) "
-				  "\\([[:digit:]]+\\) \\([[:digit:]]+\\)") o)
-	(mapcar #'string-to-number
-		(list (match-string 1 o)
-		      (match-string 2 o)
-		      (match-string 3 o)
-		      (match-string 4 o)))))))
+  (let ((page (doc-view-current-page))
+	(doc (let ((cache-doc (doc-view-current-cache-doc-pdf)))
+	       (if (file-exists-p cache-doc)
+		   cache-doc
+		 doc-view--buffer-file-name))))
+    (with-temp-buffer
+      (when (eq 0 (ignore-errors
+		    (call-process doc-view-ghostscript-program nil t
+				  nil "-dSAFER" "-dBATCH" "-dNOPAUSE" "-q"
+				  "-sDEVICE=bbox"
+				  (format "-dFirstPage=%s" page)
+				  (format "-dLastPage=%s" page)
+				  doc)))
+	(goto-char (point-min))
+	(save-match-data
+	  (when (re-search-forward
+		 (concat "%%BoundingBox: "
+			 "\\([[:digit:]]+\\) \\([[:digit:]]+\\) "
+			 "\\([[:digit:]]+\\) \\([[:digit:]]+\\)") nil t)
+	    (mapcar #'string-to-number
+		    (list (match-string 1)
+			  (match-string 2)
+			  (match-string 3)
+			  (match-string 4)))))))))
 
 (defvar doc-view-paper-sizes
   '((a4 595 842)

Message #11 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org, trent2 <at> web.de
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Tue, 13 Nov 2018 19:08:54 +0200

> From: Glenn Morris <rgm <at> gnu.org>
> Date: Mon, 12 Nov 2018 16:36:48 -0500
> Cc: 33344 <at> debbugs.gnu.org
> 
> > Please change line 1218 from
> >
> >   (format "-dFirstPage=%s -dLastPage=%s %s"
> >
> > to
> >
> >   (format "-dFirstPage=%s -dLastPage=%s \"%s\""
> 
> Thanks for the report.
> I don't see a need for the shell to be involved at all here.
> The following seems to work for me - how about for you?

Thanks, but call-process won't DTRT if the default-directory is
remote, would it?

And I wonder how many more subtle incompatibilities will such a change
cause.  All that because we need to run a single string through
shell-quote-argument (and not just enclose it in double quotes)?  Is
it really worth it?

Message #14 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org, trent2 <at> web.de
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Tue, 13 Nov 2018 13:12:40 -0500

Eli Zaretskii wrote:

> Thanks, but call-process won't DTRT if the default-directory is
> remote, would it?

s/call-process/process-file then

> And I wonder how many more subtle incompatibilities will such a change
> cause.  All that because we need to run a single string through
> shell-quote-argument (and not just enclose it in double quotes)?  Is
> it really worth it?

External processes should not be called through a shell unless they
really need that, and I see no evidence for that here.

Message #17 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org, trent2 <at> web.de
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Tue, 13 Nov 2018 21:24:55 +0200

> From: Glenn Morris <rgm <at> gnu.org>
> Cc: trent2 <at> web.de,  33344 <at> debbugs.gnu.org
> Date: Tue, 13 Nov 2018 13:12:40 -0500
> 
> > And I wonder how many more subtle incompatibilities will such a change
> > cause.  All that because we need to run a single string through
> > shell-quote-argument (and not just enclose it in double quotes)?  Is
> > it really worth it?
> 
> External processes should not be called through a shell unless they
> really need that, and I see no evidence for that here.

I don't disagree, but that's not the point.  The point is that this
code was written to use the shell, and it works.  Turning it upside
down because it failed to quote a single argument risks introducing
bugs and backward incompatibilities for what IMO is a very small gain.

Message #20 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org, trent2 <at> web.de
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Wed, 14 Nov 2018 13:14:39 -0500

Eli Zaretskii wrote:

> I don't disagree, but that's not the point.  The point is that this
> code was written to use the shell, and it works.  Turning it upside
> down because it failed to quote a single argument risks introducing
> bugs and backward incompatibilities for what IMO is a very small gain.

I don't think there's a mystery or grand design here. People sometimes
just reach for "shell-command" when they want to run an external
process, without thinking about the details.

"sh -c STUFF" is the same as just STUFF unless STUFF relies on some
shell feature like globbing. If STUFF doesn't require any shell
features then calling it via a shell is at best inefficient and at
worst harmful (if the shell mishandles any portion of STUFF, as happens here).
It is clear by inspection that this particular call does not require
shell features, so it should not go through a shell.

I've put reviewing all such uses of shell-command in Emacs on my todo
list (but it may well never happen).

(To return to a previous point: the "doc" argument here cannot be
remote, by virtue of doc-view's cache.)

Message #23 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org, trent2 <at> web.de
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Wed, 14 Nov 2018 21:29:41 +0200

> From: Glenn Morris <rgm <at> gnu.org>
> Cc: trent2 <at> web.de,  33344 <at> debbugs.gnu.org
> Date: Wed, 14 Nov 2018 13:14:39 -0500
> 
> Eli Zaretskii wrote:
> 
> > I don't disagree, but that's not the point.  The point is that this
> > code was written to use the shell, and it works.  Turning it upside
> > down because it failed to quote a single argument risks introducing
> > bugs and backward incompatibilities for what IMO is a very small gain.
> 
> I don't think there's a mystery or grand design here. People sometimes
> just reach for "shell-command" when they want to run an external
> process, without thinking about the details.

Yes, of course.  My point, again, is that this is how it worked till
now, so it is de-facto how people are used to it.

> "sh -c STUFF" is the same as just STUFF unless STUFF relies on some
> shell feature like globbing. If STUFF doesn't require any shell
> features then calling it via a shell is at best inefficient and at
> worst harmful (if the shell mishandles any portion of STUFF, as happens here).
> It is clear by inspection that this particular call does not require
> shell features, so it should not go through a shell.

I agree, but again, that's not my point.  My point is that
shell-command and call-process/process-file are subtly different,
beyond how "sh -c" differs from invoking the program directly.  Just
auditing the code to reveal those differences is a significant job,
let alone making sure the differences do or don't matter in this case.
So I questioned the wisdom of investing such an effort (or not
investing it and risking subtle incompatibilities) for such a minor
reason.

Message #26 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Robert Spillner <trent2 <at> web.de>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 33344 <at> debbugs.gnu.org
Subject: Re: bug#33344: 26.1;
 doc-view bounding-box recognition doesn't work on path names with
 spaces
Date: Fri, 23 Nov 2018 00:47:53 +0100

Hi,

sorry for the late answer -- these mails didn't go through my spam-filter.

The patch does work.

As I'm neither an active emacs programmer, lisp developer, or security expert 
I don't have a sound opinion whether this should be the way to do this. But as 
there might be security implications with unchecked strings which are 
introduced in a shell call it is probably better not to do it. But if this 
change is part of a larger code review which is going to be introduced a long 
time frm now it might be a better idea to do the quotation-hack first and 
replace it later by a better solution so the bug get fixed for now.

Just my two cents.

> Glenn Morris wrote:
> 
> Thanks for the report.
> I don't see a need for the shell to be involved at all here.
> The following seems to work for me - how about for you?
> 

Message #29 received at 33344 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Robert Spillner <trent2 <at> web.de>
Cc: Glenn Morris <rgm <at> gnu.org>, 33344 <at> debbugs.gnu.org
Subject: Re: bug#33344: 26.1; doc-view bounding-box recognition doesn't work
 on path names with spaces
Date: Wed, 26 Aug 2020 14:38:02 +0200

Robert Spillner <trent2 <at> web.de> writes:

> sorry for the late answer -- these mails didn't go through my spam-filter.
>
> The patch does work.

Thanks for testing.

I've now applied Glenn's patch to Emacs 28 (but used process-file, as
suggested, instead).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.