* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
@ 2018-11-11 11:36 Robert Spillner
2018-11-12 21:36 ` Glenn Morris
0 siblings, 1 reply; 9+ messages in thread
From: Robert Spillner @ 2018-11-11 11:36 UTC (permalink / raw)
To: 33344
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!
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-11 11:36 bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces Robert Spillner
@ 2018-11-12 21:36 ` Glenn Morris
2018-11-13 17:08 ` Eli Zaretskii
2018-11-22 23:47 ` Robert Spillner
0 siblings, 2 replies; 9+ messages in thread
From: Glenn Morris @ 2018-11-12 21:36 UTC (permalink / raw)
To: Robert Spillner; +Cc: 33344
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-12 21:36 ` Glenn Morris
@ 2018-11-13 17:08 ` Eli Zaretskii
2018-11-13 18:12 ` Glenn Morris
2018-11-22 23:47 ` Robert Spillner
1 sibling, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2018-11-13 17:08 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33344, trent2
> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-13 17:08 ` Eli Zaretskii
@ 2018-11-13 18:12 ` Glenn Morris
2018-11-13 19:24 ` Eli Zaretskii
0 siblings, 1 reply; 9+ messages in thread
From: Glenn Morris @ 2018-11-13 18:12 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33344, trent2
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-13 18:12 ` Glenn Morris
@ 2018-11-13 19:24 ` Eli Zaretskii
2018-11-14 18:14 ` Glenn Morris
0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2018-11-13 19:24 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33344, trent2
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-13 19:24 ` Eli Zaretskii
@ 2018-11-14 18:14 ` Glenn Morris
2018-11-14 19:29 ` Eli Zaretskii
0 siblings, 1 reply; 9+ messages in thread
From: Glenn Morris @ 2018-11-14 18:14 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 33344, trent2
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.)
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-14 18:14 ` Glenn Morris
@ 2018-11-14 19:29 ` Eli Zaretskii
0 siblings, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2018-11-14 19:29 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33344, trent2
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces
2018-11-12 21:36 ` Glenn Morris
2018-11-13 17:08 ` Eli Zaretskii
@ 2018-11-22 23:47 ` Robert Spillner
2020-08-26 12:38 ` Lars Ingebrigtsen
1 sibling, 1 reply; 9+ messages in thread
From: Robert Spillner @ 2018-11-22 23:47 UTC (permalink / raw)
To: Glenn Morris; +Cc: 33344
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?
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-26 12:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-11 11:36 bug#33344: 26.1; doc-view bounding-box recognition doesn't work on path names with spaces Robert Spillner
2018-11-12 21:36 ` Glenn Morris
2018-11-13 17:08 ` Eli Zaretskii
2018-11-13 18:12 ` Glenn Morris
2018-11-13 19:24 ` Eli Zaretskii
2018-11-14 18:14 ` Glenn Morris
2018-11-14 19:29 ` Eli Zaretskii
2018-11-22 23:47 ` Robert Spillner
2020-08-26 12:38 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).