bugGNU roff - Bugs: bug #58206, [PATCH] fix PDFPIC issue with...

 
 

bug #58206: [PATCH] fix PDFPIC issue with determining size of pdfs containing images

Submitter:  None
Submitted:  Sun 19 Apr 2020 06:53:50 PM UTC
   
 
Category:  Macro - others/general Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 21 Apr 2023 06:16:56 PM UTC, comment #24: 

comment #23:

> Aha.  It sounds like I should break this issue out from the
> Solaris material (in "PROBLEMS") and cover it like gdiffmk.


This was done in commit 6ee42a18.  Thanks!

Dave <barx>
Group Member
Thu 20 Apr 2023 06:29:56 AM UTC, comment #23: 

Aha.  It sounds like I should break this issue out from the Solaris material (in "PROBLEMS") and cover it like gdiffmk.

Thanks, Dave!

G. Branden Robinson <gbranden>
Group administrator
Thu 20 Apr 2023 05:57:35 AM UTC, comment #22: 

And here's an example of it failing on an earlier version of macOS.  (As you note, there's no way to glean the version of sed, but it's the stock one bundled with OS X 10.11.6.)

$ uname -a
Darwin Ignatius 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64 i386
$ sed --version # reveal non-GNU sed
sed: illegal option -- -
usage: sed script [-Ealn] [-i extension] [file ...]
       sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...]
$ echo foo | sed 's/foo/&\nbar/'
foonbar


Dave <barx>
Group Member
Wed 19 Apr 2023 06:51:16 PM UTC, comment #21: 

Here is an illustration of "\n" working as an encoded newline as replacement text in macOS sed's 's' command.


$ uname -a
Darwin gcc104.fsffrance.org 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:20:05 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T8101 arm64
$ sed --version # reveal non-GNU sed
sed: illegal option -- -
usage: sed script [-Ealnru] [-i extension] [file ...]
        sed [-Ealnu] [-i extension] [-e script] ... [-f script_file] ... [file ...]
$ echo foo | sed 's/foo/&\nbar/'
foo
bar


G. Branden Robinson <gbranden>
Group administrator
Wed 19 Apr 2023 06:06:29 PM UTC, comment #20: 


comment #19:

> comment #11:
> > the sed command referenced in comment #10 will fail on at least > Mac OS: its replacement pattern relies on "\n" to output a newline,
>
> Comment 2 of bug #64061 exposes this as the dirty lie it is.


Hold that thought!  I'm still getting my ducks lined up (meaning: checking portability of my working copy changes on GNU/Linux, macOS, and Solaris 10 & 11).

There is an evil portability problem, and it lies where we would not have expected it.  More to follow in #64061.

But to not keep you in too much suspense: "\n" in the sed s command's replacement text is (1) non-standard, (2) supported by macOS sed anyway, and (3) necessary for pdfpic.tmac to work.

G. Branden Robinson <gbranden>
Group administrator
Wed 19 Apr 2023 03:49:16 PM UTC, comment #19: 

comment #11:

> the sed command referenced in comment #10 will fail on at least > Mac OS: its replacement pattern relies on "\n" to output a newline,


Comment 2 of bug #64061 exposes this as the dirty lie it is.

Dave <barx>
Group Member
Wed 26 Jan 2022 07:01:12 AM UTC, comment #18: 

Bug #61324 was closed as a dup of this bug.

Dave <barx>
Group Member
Sun 23 Jan 2022 04:27:23 AM UTC, comment #17: 

comment #15:

> I misread the output--the problem file from the original submission indeed contains UTF-16LE, not surprising at all given all the Windows systems in the world.


Embarrassingly, I had it right the first time.  A null (zero) byte followed by a byte at a higher address with its eighth bit clear, representing a Unicode basic Latin character, is definitely big-endian.

I'll have to send some of whatever I was munching on to Michael Pollan.

G. Branden Robinson <gbranden>
Group administrator
Fri 21 Jan 2022 10:53:33 AM UTC, comment #16: 


commit 1483e1168224e9d37ff0986ccf5e3346af7e9281
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Jan 21 20:38:19 2022 +1100

    [pdfpic]: Fix Savannah #58206.

    * tmac/pdfpic.tmac (PDFPIC): Scrub null bytes out of pdfinfo(1) output.
      Thanks to an anonymous contributor for the patch (the commentary about
      it is mine, if someone wants an argument).

    Fixes <https://savannah.gnu.org/bugs/?58206>.

commit 3f094e31225d0a22b250afedf4ee13f3902bd691
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Fri Jan 21 20:27:42 2022 +1100

    [pdfpic]: Regression-test Savannah #58206.

    * tmac/tests/pdfpic_does-not-choke-on-bad-pdfinfo-output.sh: Do it.
    * tmac/tmac.am (tmac_TESTS): Run test.

    Test fails at this commit.


G. Branden Robinson <gbranden>
Group administrator
Fri 21 Jan 2022 08:59:38 AM UTC, comment #15: 

I misread the output--the problem file from the original submission indeed contains UTF-16LE, not surprising at all given all the Windows systems in the world.

And for those who were wondering, I've worked out a tempfile-free way to manipulate the within-build-tree gnu.eps file into an afflicted gnu.pdf that reproduces this problem.

So I can get my regression test now.



$ gs -o - -sDEVICE=pdfwrite -f build/gnu.eps -c "[ /Title (\000B\000U\000S\000T\000E\000D) /DOCINFO pdfmark" | pdfinfo - | xxd | head
00000000: 5469 746c 653a 2020 2020 2020 2020 2020  Title:
00000010: 0042 0055 0053 0054 0045 0044 0a43 7265  .B.U.S.T.E.D.Cre
00000020: 6174 6f72 3a20 2020 2020 2020 2070 6e6d  ator:        pnm
00000030: 746f 7073 0a50 726f 6475 6365 723a 2020  tops.Producer:
00000040: 2020 2020 2047 504c 2047 686f 7374 7363       GPL Ghostsc
00000050: 7269 7074 2039 2e32 370a 4372 6561 7469  ript 9.27.Creati
00000060: 6f6e 4461 7465 3a20 2020 4672 6920 4a61  onDate:   Fri Ja
00000070: 6e20 3231 2031 393a 3538 3a33 3820 3230  n 21 19:58:38 20
00000080: 3232 2041 4544 540a 4d6f 6444 6174 653a  22 AEDT.ModDate:
00000090: 2020 2020 2020 2020 4672 6920 4a61 6e20          Fri Jan


G. Branden Robinson <gbranden>
Group administrator
Fri 21 Jan 2022 06:31:36 AM UTC, comment #14: 

I'm mostly unblocked.

The problem with the original problematic file (the "angular 1200x800" thing) appears to be that it had a title property that was encoded in UTF-16BE.


$ xxd angular-1280-800.pdf | sed -n '/459f0/,/45a30/p'
000459f0: 3c3c 0a2f 5469 746c 6520 3c30 3036 3130  <<./Title <00610
00045a00: 3036 4530 3036 3730 3037 3530 3036 4330  06E00670075006C0
00045a10: 3036 3130 3037 3230 3032 4430 3033 3130  0610072002D00310
00045a20: 3033 3230 3033 3830 3033 3030 3032 4430  03200380030002D0
00045a30: 3033 3830 3033 3030 3033 3030 3030 303e  038003000300000>


You don't see that a lot these days, with the success of the global campaign to exterminate big-endian desktop (and mobile) computing.

So this is what pdfinfo ends up doing with that.


$ pdfinfo angular-1280-800.pdf | xxd
00000000: 5469 746c 653a 2020 2020 2020 2020 2020  Title:
00000010: 0061 006e 0067 0075 006c 0061 0072 002d  .a.n.g.u.l.a.r.-
00000020: 0031 0032 0038 0030 002d 0038 0030 0030  .1.2.8.0.-.8.0.0
00000030: 0000 0a50 726f 6475 6365 723a 2020 2020  ...Producer:
00000040: 2020 2068 7474 7073 3a2f 2f69 6d61 6765     https://image
00000050: 6d61 6769 636b 2e6f 7267 0a43 7265 6174  magick.org.Creat
00000060: 696f 6e44 6174 653a 2020 204d 6f6e 2041  ionDate:   Mon A
00000070: 7072 2032 3020 3034 3a33 333a 3434 2032  pr 20 04:33:44 2
00000080: 3032 3020 4145 5354 0a4d 6f64 4461 7465  020 AEST.ModDate
00000090: 3a20 2020 2020 2020 204d 6f6e 2041 7072  :        Mon Apr
000000a0: 2032 3020 3034 3a33 333a 3434 2032 3032   20 04:33:44 202
000000b0: 3020 4145 5354 0a54 6167 6765 643a 2020  0 AEST.Tagged:
000000c0: 2020 2020 2020 206e 6f0a 5573 6572 5072         no.UserPr
000000d0: 6f70 6572 7469 6573 3a20 6e6f 0a53 7573  operties: no.Sus
000000e0: 7065 6374 733a 2020 2020 2020 206e 6f0a  pects:       no.
000000f0: 466f 726d 3a20 2020 2020 2020 2020 2020  Form:
00000100: 6e6f 6e65 0a4a 6176 6153 6372 6970 743a  none.JavaScript:
00000110: 2020 2020 206e 6f0a 5061 6765 733a 2020       no.Pages:
00000120: 2020 2020 2020 2020 310a 456e 6372 7970          1.Encryp
00000130: 7465 643a 2020 2020 2020 6e6f 0a50 6167  ted:      no.Pag
00000140: 6520 7369 7a65 3a20 2020 2020 2031 3238  e size:      128
00000150: 3020 7820 3830 3020 7074 730a 5061 6765  0 x 800 pts.Page
00000160: 2072 6f74 3a20 2020 2020 2020 300a 4669   rot:       0.Fi
00000170: 6c65 2073 697a 653a 2020 2020 2020 3238  le size:      28
00000180: 3539 3337 2062 7974 6573 0a4f 7074 696d  5937 bytes.Optim
00000190: 697a 6564 3a20 2020 2020 206e 6f0a 5044  ized:      no.PD
000001a0: 4620 7665 7273 696f 6e3a 2020 2020 312e  F version:    1.
000001b0: 330a                                     3.


In other words, it simply blasts the encoded bytes to its own output in utter indifference to the character encoding used by the output device.  For an information-extraction tool whose entire purpose is human-readable output, that seems a dubious decision to me.

But, we're stuck with it for the time being (unless a PDFPIC user wants to migrate to Deri's lower-level output driver-leveraging alternative in comment #7).

I'll see if I can force a UTF-16 Title property onto gnu.eps so that I can craft a proper regression test.

G. Branden Robinson <gbranden>
Group administrator
Sat 18 Dec 2021 06:11:25 AM UTC, comment #13: 

The main barrier to proceeding with this is less the lack of portable scriptage to throw at it than a nice, small, GPL-compatibly-licensed, image-bearing PDF we can use to incorporate the fix into our test suite--this would help detect if poppler changes its behavior on us.

I'll see if I can contrive one out of 'gnu.eps' (already generated as part of the build), but if I don't update this ticket soon, a contribution would be appreciated--at present I haven't yet rotated off of me(7)'s bug list.

G. Branden Robinson <gbranden>
Group administrator
Fri 17 Dec 2021 07:25:33 PM UTC, comment #12: 

Another awk solution, similar to the one linked in comment #10, was developed independently (http://lists.gnu.org/r/groff/2021-12/msg00002.html).  A patch is attached in file #52513.

Dave <barx>
Group Member
Mon 08 Nov 2021 10:16:18 AM UTC, comment #11: 

Also, as written, the sed command referenced in comment #10 will fail on at least Mac OS: its replacement pattern relies on "\n" to output a newline, and a mailing list thread on an unrelated topic (http://lists.gnu.org/r/groff/2021-10/msg00066.html) points out that this is not a POSIX usage and does not work on Mac's standard sed.

Dave <barx>
Group Member
Wed 03 Nov 2021 01:34:28 PM UTC, comment #10: 

Addressing bug #55107 will obviate the need for this particular shell pipeline at all (though tmac/pdfpic.tmac contains several other .sy calls which may still persist).  But that's a more substantial change, and the patch proposed in comment #4 here is simple and low-risk, so it's something that could be done until 55107 is addressed.

The mailing list recently discussed alternative fixes to this pipeline, including replacing the problematic grep with an awk (http://lists.gnu.org/archive/html/groff/2021-09/msg00061.html) or sed command (http://lists.gnu.org/archive/html/groff/2021-09/msg00062.html).  But the comment #4 patch looks to me like the simplest and most portable fix.

Dave <barx>
Group Member
Sat 17 Oct 2020 10:22:26 PM UTC, comment #9: 


comment #8:

> This isn't really in progress, but I don't promise I won't get back to it during the 1.22.3 RC cycle.


I meant "1.23 RC cycle".  I can promise I won't get anything done for the 1.22.3 RC cycle unless a DeLorean or police call box falls into my possession.

G. Branden Robinson <gbranden>
Group administrator
Fri 16 Oct 2020 11:58:17 PM UTC, comment #8: 

This isn't really in progress, but I don't promise I won't get back to it during the 1.22.3 RC cycle.

G. Branden Robinson <gbranden>
Group administrator
Tue 21 Apr 2020 11:00:23 PM UTC, comment #7: 

PDFPIC is not written by me, but I understand why it was written! The gropdf driver supports this:-

\X’pdf: pdfpic file alignment width height line-length’

Place an image of the specified width containing the PDF drawing from file file of desired width
and height (if height is missing or zero then it is scaled proportionally). If alignment is -L the
drawing is left aligned. If it is -C or -R a linelength greater than the width of the drawing is re-
quired as well. If width is specified as zero then the width is scaled in proportion to the height.

The problem with this low level command is that the position where groff will render next is not altered, gropdf has no way of signalling back to groff the vertical space occupied by the picture. So it is down to the author to move down the required distance, or set an indent if the text is to flow to the right of the picture, adding a trap to remove the indent when the bottom the picture is reached.

Here's a simple example using your bad pdf. It requires that you know the x/y proportions, in your case 1200x800 pixels it is 3/2:-

Here it is:-
\#.PDFPIC Bad3..pdf
\# Alternatively ...
.nr len 4i
\X'pdf: pdfpic Bad.pdf -L \n[len]z'
\# Back to top of picture
.sp -1l
\# Pic is 1200x800, so if width is 4i length would be 4i * 2/3
.sp \n[len]u*2u/3u
And here we are.

You don't need to groff -U so it perfectly safe.

Deri James <deri>
Group Member
Tue 21 Apr 2020 08:12:22 PM UTC, comment #6: 

I found a similar issue in the their issue tracker sadly it is still unresolved.

https://gitlab.freedesktop.org/poppler/poppler/-/issues/776

The argument for supporting PDFPIC is to allow files contain pdfmark's pdfhref feature can also contain pictures. If a file that contains pdfhref's is converted to postscript then only the text remains but no link is available even after converting it to a pdf using ps2pdf.

Anonymous
Tue 21 Apr 2020 01:01:33 PM UTC, comment #5: 


comment #4:

> Ok so a work around I used was to use tr's -d flag to remove the NULL sections from the output from pdfinfo. As far as I know this should be available to all systems. Another option is to use cat -v but I don't think that is POSIX complaint.


Indeed it is not.

POSIX cat admits only -u, and even that is too much for some old-timers.

tr -d does indeed seem fine.  Would someone like to report a bug to poppler upstream?  In my opinion is not Unixy for a "document information extractor" to spew a binary stream to standard output unless this is explicitly requested.

Note that the poppler-utils package in Debian stable is 0.71, and the latest upstream release is 0.87 (made at the end of March), so it might be polite to trawl the bug tracker to see if the issue has already been resolved.

https://gitlab.freedesktop.org/poppler/poppler/issues

I share Ingo's discomfort at all this `.sy`ing, but at least someone has to turn the safety off of the gun with -U first before the macro will work.

I'm inclined to apply the patch (and add a comment justifying the perhaps-surprising use of tr -d '\000', while the provenance and continued value of .PDFPIC can be determined.  Our contributor Deri James knows vastly more about PDF integration than I do.

I'll put the bug state into "In Progress".  I need to reproduce it--with luck this is easy with in-groff-source-tree materials, and that will tell me how it easy it is to write a regression test for it.

G. Branden Robinson <gbranden>
Group administrator
Tue 21 Apr 2020 02:34:03 AM UTC, comment #4: 

Ok so a work around I used was to use tr's -d flag to remove the NULL sections from the output from pdfinfo. As far as I know this should be available to all systems. Another option is to use cat -v but I don't think that is POSIX complaint.

Here is the diff

diff --git a/tmac/pdfpic.tmac b/tmac/pdfpic.tmac
index 0400c1cf..3dae30be 100644
--- a/tmac/pdfpic.tmac
+++ b/tmac/pdfpic.tmac
@@ -84,6 +84,7 @@
 .\" get image dimensions
 .  ec @
 .  sy pdfinfo @$1 | \
+tr -d '\000' | \
 grep "Page *size" | \
 sed -e 's/Page *size: *\\([[:digit:].]*\\) *x *\\([[:digit:].]*\\).*$/\
 .nr pdf-wid (p;\\1)\\n\


(file #48890)

Anonymous
Tue 21 Apr 2020 02:07:04 AM UTC, comment #3: 

I just attached the pdf i was trying to embed. This was mostly just to see if pdfpic was working.

Anonymous
Mon 20 Apr 2020 03:18:54 AM UTC, comment #2: 

While nowadays, grep(1) -a tends to be universally available on all Linux and BSD systems, notable systems that do not support it include Illumos and Oracle Solaris (even the newest version 11.3).  So Branden's remark about portability is not a theoretical concern.  While Illumos and Solaris don't appear to ship groff by default, using groff there doesn't seem uncommon, so i'd consider them relevant target platforms, and the patch seems likely to totally break pdfpic.tmac on these platforms.

The Solaris versions of grep seem to simply fail on binary files; when called with -F, they seem to simply pass through non-printable characters, but we can't use -F here because '*' is needed.  The Solaris manual page does not mention any way to handle binary files.

The normal way to include images into roff(7) files is to convert them to eps format and then use the .PSPIC macro.

Using the .PDFPIC macro looks like a bad idea in the first place.  It is highly insecure because it makes copious use auf the .sy request.  Also, it is almost undocumented: all i managed to find so far is a passing mention in groff_tmac(7), which is riddled with very confusing typos - it talks about defining PSPIC, and about PSDIF options, neither of which make sense.  The info(1) documentation doesn't seem to mention .PDFPIC it at all.

I'm wondering though how it can happen that the output from pdfinfo(1) contains non-printable characters...  Maybe this is a bug in pdfinfo(1), not in groff?  Can you show the PDF file that triggered the problem for you?

Ingo Schwarze <schwarze>
Group Member
Sun 19 Apr 2020 10:51:25 PM UTC, comment #1: 

Unfortunately, the '-a' option to grep is not part of the POSIX standard for the utility so we can't rely on it being available in the runtime environment.

Of course, 'pdfinfo' isn't standard at all, so there's probably some room to argue here.

G. Branden Robinson <gbranden>
Group administrator
Sun 19 Apr 2020 06:53:50 PM UTC, original submission:  

When .PDFPIC is used it calls to pdfinfo piped to grep to determine the dimensions of the pdf. However when the pdf contains an image grep fails to parse the date giving the following error.


Binary file (standard input) matches


Allowing grep to process Binary files seems to fix the issue. Below is the patch.


diff --git a/tmac/pdfpic.tmac b/tmac/pdfpic.tmac
index 0400c1cf..4bc6f03b 100644
--- a/tmac/pdfpic.tmac
+++ b/tmac/pdfpic.tmac
@@ -84,7 +84,7 @@
 .\" get image dimensions
 .  ec @
 .  sy pdfinfo @$1 | \
-grep "Page *size" | \
+grep -a "Page *size" | \
 sed -e 's/Page *size: *\\([[:digit:].]*\\) *x *\\([[:digit:].]*\\).*$/\
 .nr pdf-wid (p;\\1)\\n\
 .nr pdf-ht  (p;\\2)/' \


Should be a relatively easy fix.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #48890:  trversion.diff added by None (342B - text/x-patch)
file #48889:  angular-1280-800.pdf added by None (279KiB - application/pdf - Here is the pdf file I used)
file #48873:  pdfpic.diff added by None (375B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by deri (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-01-21 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2022-01-21 gbranden StatusNeed Info In Progress
    2021-12-18 gbranden StatusNone Need Info
    2020-10-16 gbranden StatusIn Progress None
    2020-04-21 gbranden StatusNeed Info In Progress
        Assigned toNone gbranden
    2020-04-21 None Attached File- Added trversion.diff, #48890
    2020-04-21 None Attached File- Added angular-1280-800.pdf, #48889
    2020-04-20 schwarze CategoryDriver gropdf Macro - others/general
        StatusNone Need Info
    2020-04-19 None Attached File- Added pdfpic.diff, #48873

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code