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

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

[Robert Spillner]

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!

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

[Glenn Morris]

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)

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Mon, 12 Nov 2018 16:36:48 -0500
> Cc: 33344@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?

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

[Glenn Morris]

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.

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: trent2@web.de,  33344@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.

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

[Glenn Morris]

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.)

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: trent2@web.de,  33344@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.

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

[Robert Spillner]

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?
> 

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

[Lars Ingebrigtsen]

Robert Spillner <trent2@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