bugGNU roff - Bugs: bug #64576, [pdf.tmac] pdf*href option...

 
 

bug #64576: [pdf.tmac] pdf*href option handling insufficiently flexible

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Mon 21 Aug 2023 09:50:40 AM UTC
   
 
Category:  Macro - others/general Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  None
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 10 Apr 2024 08:34:38 PM UTC, comment #9: 

comment #2:

> I reiterate that stuff like this:


> .      while dpdf:href.opt\\$1 \{\
> .         pdf:href.opt\\$1 \\$@
> .         shift \\n[pdf:href.argc]
> .         \}


> is too clever by half--by three quarters, maybe,

This, of course, is utter nonsense.

> if you're neither doing sanitization (which will need to be
> reversible for things like link text, and therefore even more
> tedious and difficult than sanitization already is), nor input
> validation.
>
> But the latter isn't even really what you want anyway.

You got that much right.

> There's no reason that every argument to a macro must be expected
> to be a valid *roff identifier.

Also true; however...

> This is simply a mistaken premise that appears to have driven
> several bad design decisions.

This conclusion is, once again, utter nonsense.  The design is perfectly sound; it is the implementation that is flawed.  You even hinted at the fault, and thus implicitly at a practical solution, in your reference to "input validation", and that is readily accomplished:

.      while \A'\\$1' \{\
.         if !dpdf:href.opt\\$1 .break
.         pdf:href.opt\\$1 \\$@
.         shift \\n[pdf:href.argc]
.         \}

will prevent any attempt to match any argument which could not be legitimately incorporated into an identifier name, (and any which follow it), to a defined macro name, (which is the intent of this while loop).

I have identified six loops, within pdfmark.pdf, which exhibit such vulnerability, and thus, where similar input validation would be advisable; I will fix them accordingly, in my OSDN groff-pdfmark repository.

Keith Marshall <keithmarshall>
Group Member
Tue 05 Sep 2023 06:15:00 PM UTC, comment #8: 


> Incidentally, and to motivate my frustration with the `pdfhref` approach, the following works terribly.  The hotspots don't know where to end, going nuts and electrifying everything after them even without a page break to screw them up by not calling `pdfmarksuspend` and `pdfmarkrestart`.


I have had a look at this now. Your runaway hot spotting was cured by removing the \c from the \\*[an*hyperlink]\c after the "--". However, this just hotspots the hyperlink and the text in the diversion is output as normal text, which is not what we want!

The problem is that it is difficult to pass the contents of a diversion as a parameter to pdfhref, so I added a new facility to pdf.tmac that if you pass the pipe character "|" on its own as the description then the hotspot is started but it is then the responsibility of the caller to mark the end of the hotspot by calling:-

.        nop \X'pdf: markend'\m[\\*[pdf:curcol]]\c

Which will also restore the colour before hotspotting started. At the moment, I have only added this facility to pdfhref-L but I'd consider extending if needed. I also have not added pdfmarksuspend/restart to man footer/header definitions, I'm not that familiar with the man macro set.

The attached gropdf.1.pdf has three hotspots

(file #55114)

Deri James <deri>
Group Member
Sun 27 Aug 2023 07:49:50 PM UTC, comment #7: 

Apologies, the t.pdf file had a problem. It worked fine with okular and evince, but some other viewers did not make the links clickable. I have fixed the issue and t-new.pdf should be working with all viewers.

(file #55090)

Deri James <deri>
Group Member
Sat 26 Aug 2023 08:12:18 PM UTC, comment #6: 

I've just lost half a days typing, my fault, closed the wrong firefox window! Not all is lost, phew, found snippets in my clipboard history as I copied text from Dasher to the savannah page, but it has not got everything, because some I typed directly. I have attached an example pdf and its source which illustrates the fix.

The information below was going to be a reply to Branden & Alex, regarding a fourth parameter to the .MR macro, which seemed to be misunderstood, but it never got finished. Some of it may be useful information in this bug report, but it was written before I had implemented the interim fix for this bug report.




> I expect man page authors would violently protest if they were told they
> had to type all those quotes and, worse, repeat the name of the page.


The fourth parameter is never required by the man page author, and definitely
should not be used.

There is a fundamental difference between .MR used in a terminal viewer, where
it becomes an external reference to another man page (man://...), and pdf
output where it is an internal reference to a man entry in the same document,
but only if the man entry exists, otherwise it is just treated as text rather
than a clickable. So if you produce a single man page as a pdf it will have no
clickables within it, unless it references itself as a "See also"!

It only becomes more useful when dealing with a collection of man entries
within a single pdf, since then references between entries become clickable.
To make this clear: with pdf output, .MR does nothing unless it is being used to create a collated collection of man pages.

Talking about internal links (i.e. .pdfhref L).

The basic problem goes back to the design of the pdfmark macros. The concept
is simple, one macro call marks a particular location in a document (.pdfhref
M) which accept two strings, a name and a descriptive text, either of which
can be omitted, and a further macro which creates a link to a named
destination (.pdfhref L) which again accepts the same two strings, either of
which can be omitted.

When name is omitted the first word of descriptive text is used as the "name".
Here's the first issue "descriptive text" is intended to become part of the
text flow of the document, so may contain typographic elements, so the first
"word" (including typographic elements) would become the name. The second
issue is that if descriptive text is missing from the link (.pdfhref L), the
descriptive text from the matching .pdfhref M is used. Note also that the link
may appear before the mark within the document, which is why pdfroff and
pdfmom do multiple runs of groff, the first run creates a string variable
based on the mark’s given name containing it's descriptive text and outputs a
.ds line via a .tm which is then fed back into the next groff run before the
actual document. This is where the problem lies. If the computed string
variable name contains anything other than straight text this may be an
illegal name for a string variable.

So .MR is the equivalent of calling .pdfhref L without a separate "name"
provided, the 4th parameter rectifies this by separating name and descriptive
text. Given .MR is only active with man page collections, and a collection
will probably require a make file or some other script it is this which can
add the 4th parameter, not the man page author.

Hopefully, Branden's new "for" command will solve the issue, but I will wait
to see what it returns if the whole string is made of special characters, such
as when the document is written in a different language and run through
preconv.




Now to turn to external links, such as Branden's experiment with .pdfhref W. This again has been fixed to stop groff complaining, however, his example will not work using the special characters from the SS fonts. It is much more likely that an IDN will come from a UTF-8  document written in greek. The example shows this "working". The browser fails to locate the domain, but the address looks correct in the browser window.


(file #55085, file #55086)

Deri James <deri>
Group Member
Tue 22 Aug 2023 04:00:22 PM UTC, comment #5: 

comment #4:

> Just completed a fix to allow any input to be used as a valid *roff identifier. This allows all UTF-8 to be used as well. So both "Known Issues" in my NewGropdf document are now ticked off. :-)


Wow!  Push, push!  (As Herbie Mann said.  Or Robert Plant.)  I want to see what this looks like when we don't have `for` yet!

Incidentally, and to motivate my frustration with the `pdfhref` approach, the following works terribly.  The hotspots don't know where to end, going nuts and electrifying everything after them even without a page break to screw them up by not calling `pdfmarksuspend` and `pdfmarkrestart`.


diff --git a/tmac/an.tmac b/tmac/an.tmac
index ad9090ceb..f614cef03 100644
--- a/tmac/an.tmac
+++ b/tmac/an.tmac
@@ -1110,6 +1110,9 @@ contains unsupported escape sequence
 .    ie \\n[dn] \{\
 .      if \\n[an*is-output-html] \
 .        nop \X^html:<a href="\\*[an*prefix]\\*[an*hyperlink]">^\c
+.      if \\n[an*is-output-pdf] \
+.        pdfhref W -D \\*[an*prefix]\\*[an*hyperlink] -- \
+\\*[an*hyperlink]\c
 .      if \\n[an*is-output-terminal] \
 .        nop \X^tty: link \\*[an*prefix]\\*[an*hyperlink]^\c
 .      \" Strip off the final newline of the diversion and emit it.
@@ -1127,6 +1130,9 @@ contains unsupported escape sequence
 .      if \\n[an*is-output-html] \
 .        nop \X^html:<a href="\\*[an*prefix]\\*[an*hyperlink]">\
 \\*[an*hyperlink]</a>^\c
+.      if \\n[an*is-output-pdf] \
+.        pdfhref W -D \\*[an*prefix]\\*[an*hyperlink] -- \
+\\*[an*hyperlink]\c
 .      if \\n[an*is-output-terminal] \
 .        nop \X^tty: link \\*[an*prefix]\\*[an*hyperlink]^\
 \\*[an*hyperlink]\X^tty: link^\c


G. Branden Robinson <gbranden>
Group administrator
Tue 22 Aug 2023 03:55:47 PM UTC, comment #4: 

Just completed a fix to allow any input to be used as a valid *roff identifier. This allows all UTF-8 to be used as well. So both "Known Issues" in my NewGropdf document are now ticked off. :-)

Deri James <deri>
Group Member
Tue 22 Aug 2023 03:26:28 PM UTC, comment #3: 

Not anticipated for my next push.

G. Branden Robinson <gbranden>
Group administrator
Mon 21 Aug 2023 12:27:51 PM UTC, comment #2: 

I reiterate that stuff like this:


.      while dpdf:href.opt\\$1 \{\
.         pdf:href.opt\\$1 \\$@
.         shift \\n[pdf:href.argc]
.         \}


is too clever by half--by three quarters, maybe, if you're neither doing sanitization (which will need to be reversible for things like link text, and therefore even more tedious and difficult than sanitization already is), nor input validation.

But the latter isn't even really what you want anyway.  There's no reason that every argument to a macro must be expected to be a valid *roff identifier.  This is simply a mistaken premise that appears to have driven several bad design decisions.

G. Branden Robinson <gbranden>
Group administrator
Mon 21 Aug 2023 12:23:45 PM UTC, comment #1: 


original submission:

> An immense amount of code in pdf.tmac seems to be dedicated to an exploration of the question "hey, what if we chucked established roff programming idioms out the window and re-implemented getopt_long(3) in it so that shell script programmers had macro interfaces that looked vaguely familiar"?


This indeed seems to be the problem--bad interface design.

The problem is that `pdfhref` doesn't require, and internal macros like `pdf*href` doesn't expect, operand-style arguments in its argument list that aren't valid roff identifiers--that is, ones that aren't preceded by an option argument.

I prepared an example based on the first figure in Wikipedia's "Internationalized domain name" article, figuring that this would be adequately demonstrative of real-world use.


$ cat EXPERIMENTS/pdfhref-whack.groff
Hello
.pdfhref W -D http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr \
http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr


Here's an annotated run of that.


$ ./build/test-groff -a -ww -b -Tpdf EXPERIMENTS/pdfhref-whack.groff
<beginning of page>
GBR: pdfhref: arguments: "W" "-D" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr"
GBR: pdfhref: checking existence of string "pdf*hrefW"
GBR: pdfhref: checking existence of string "pdf*href-W"
GBR: pdfhref: calling pdf*href "-D" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr"
GBR: pdf*href: arguments: "-D" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr"
GBR: pdf*href: while dpdf:href.opt-D
GBR: pdf*href: argument list is now "-D" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr"
GBR: pdf*href: .pdf:href.opt-D "-D" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr" "http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr"
GBR: pdf:href.opt-D: ds pdf:href-D http://\(*o\(*u\(*t\(*o\(*p\(*i\(*a.\(*d\(*p\(*h.gr
GBR: pdf:href.opt-D: nr pdf:href.argc 2
GBR: pdf:href.opt-D: outta here
GBR: pdf*href: .shift 2
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/pdf.tmac':457
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/pdf.tmac':665: while loop
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/pdf.tmac':671: macro 'pdf*href'
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/pdf.tmac':457: while loop
troff: backtrace: '/home/branden/src/GIT/groff/build/../tmac/pdf.tmac':482: macro 'pdfhref'
troff: backtrace: file 'EXPERIMENTS/pdfhref-whack.groff':3
troff:EXPERIMENTS/pdfhref-whack.groff:3: error: a special character is not allowed in an identifier
GBR: pdf*href: done with while loop
GBR: pdfhref: How did that go?
Hello http://<*o><*u><*t><*o><*p><*i><*a>.<*d><*p><*h>.gr


Long story short...


658 .\" Now we interpret, and remove any specified options from the
659 .\" argument list.  (Note that only options with a declared handler
660 .\" will be processed;  there is no provision for detecting invalid
661 .\" options -- anything which is not recognised is assumed to start
662 .\" the "link text" component of the argument list).
663 .\"
664 .tm GBR: \\$0: while dpdf:href.opt\\$1
665 .while dpdf:href.opt\\$1 \{\
666 .   tm GBR: \\$0: argument list is now \\$@
667 .   tm GBR: \\$0: .pdf:href.opt\\$1 \\$@
668 .   pdf:href.opt\\$1 \\$@
669 .   tm GBR: \\$0: .shift \\n[pdf:href.argc]
670 .   shift \\n[pdf:href.argc]
671 .   \}
672 .tm GBR: \\$0: done with while loop


...isn't up to scratch.  Sure, some option arguments have been shifted off, but the very next one might be link text, have escape sequences in it, and cause the formatter to scream when it tests to see if that argument is defined as a string/macro/diversion with the 'd' operator.

The subsequent logic is telling:


673 .\"
674 .\" If we found "--", to mark the end of the options, then we should
675 .\" discard it.
676 .\"
677 .if '\\$1'--' .shift


This doesn't cut it.  It's too late.  For this to work, '--' is going to have to be a mandatory part of the interface.

I'll see how easy that is to do.

G. Branden Robinson <gbranden>
Group administrator
Mon 21 Aug 2023 09:50:40 AM UTC, original submission:  

This code:


.\"
.\" Macros "pdf:href.flag" and "pdf:href.option"
.\" provide a generic mechanism for switching on flag type options,
.\" and for decoding options with arguments, respectively
.\"
.de pdf:href.flag
.\" ----------------------------------------------------------------------
.\" ----------------------------------------------------------------------
.nr pdf:href\\$1 1
.nr pdf:href.argc 1
..
.de pdf:href.option
.\" ----------------------------------------------------------------------
.\" ----------------------------------------------------------------------
.ds pdf:href\\$1 \\$2
.nr pdf:href.argc 2


...is insufficiently flexible.  It assume that its inputs will consist only of ordinary characters, but special characters and escape sequences, particular for callers of `pdf:href.option`, are conceivable.

For example, a macro like groff man(7)'s `UR`, when used with no link text (which is a bit lazy, but accepted), will run into problems in cases like the following.


.P
.I ps2eps
is available from CTAN mirrors, e.g.,
.UR ftp://\:ftp\:.dante\:.de/\:tex\-archive/\:support/\:ps2eps/
.UE .


That's a real example from our pic(1) page.  One approach to resolving it implies laboriously walking the arguments to macros that call `pdf:href.flag` and `pdf:href.option` (which are internals--not externally documented and therefore not an API), attempting to scrub them of unexpected content, and getting peevish with other groff developers when encountering arbitrary roff input that is unexpectedly unexpected; see, e.g., bug #64202.

That it is so tedious to iterate through strings in groff (and as I have said elsewhere, nigh-impossible in AT&T troff) is doubtless one of the factors that turns up the temperature on this problem.  See bug #62264 for a proposed, but not yet implemented, quality-of-life improvement in this area.

Another possibility is simply for pdf.tmac- or pdfmark.tmac-using documents and macro packages to be aware of the intolerance/irritability of its internals, and work around them--for instance, groff's an.tmac, when seeing that a `UR` or `MT` has no link text, could simply inject some known, well-behaved link text like "(link)", that aforementioned internals won't barf on.  This works (I tried it), but it is pretty lame.

1.  That text isn't localized.
2.  That text might not appropriate or clear in all situations.

Now, one could kick both of the above back into the user's face.  ("Just supply some link text, damn it!")  But for another problem...

3.  Worst, you can't format punctuation after it without intervening space.  To do that, you need the `\c` escape sequence, which becomes part of one of `pdfhref`'s arguments, and pdfmark.tmac / pdf.tmac insist on populating roff register or string names incorporating each such argument, and we're back to the original problem of escape sequences.


troff:<standard input>:1473: error: an escaped 'c' is not allowed in an identifier


And in fact use of `\c` is wholly defeated here--you'll get space (and possibly a break) before the punctuation anyway.  So tossing the burden of specifying link text--which is supposed to be formatted output in the first place--on the user and then going aggro on them if they dare to use escape sequences that are wholly valid in formatted output is not a satisfactory solution.

Intriguingly, the `\A` escape sequence to test a character sequence for validity as a groff identifier name has been around since 1991, but pdfmark.tmac and pdf.tmac don't bother to use it.  Possibly this problem would have been recognized and addressed long ago if they had.  It certainly seems to me like a Recommended Best Practice if one is going to be populating groff identifiers based on user input (or even any external input, like a macro package written by someone who isn't as careful as you are).  But nobody ever got a fellowship for validating input, did they?

Moreover, it appears that the main reason pdfmark.tmac / pdf.tmac are taking this approach is because the roff language doesn't have a list type, so it's a pain in the ass to search for things.  pdfmark.tmac / pdf.tmac's solution, to use the macro/request/string name space as a dictionary, with the identifiers as keys and the string contents as values, does have obvious appeal given that limitation...but for blundering into the other limitations of assuming either that (a) any input makes a valid identifier, or (b) your users won't wander off the lit path of ordinary characters.  And as noted above, scrubbing a character sequence for things that are invalid (in any context)--the "sanitiziation problem", is Yet Another pain in the ass.  See bug #62264 again.

Fortunately, the use of this mechanism, in pdf.tmac at least, appears to be fairly limited.

`pdf.href.flag` would seem to be okay, since its values only ever come from macro arguments that identify "flags", and these are going to have straightforward names.

For instance, these seem okay (includes annotations from my working copy).


671 .\" XXX: predefined flag
672 .if !dpdf:href-D .pdf:href.option -D \\$1
673 .if '\\*[pdf:href-D]'' \{\
674 .   pdf:error pdfhref has no destination
675 .   nr pdf:href.ok 0
676 .   \}

690 .\" XXX: predefined flag
691 .if dpdf:href-P \&\\*[pdf:href-P]\c
692 .ie \\n[pdf:href.ok] \{\
693 .   \"
[~40 lines of brace scope follow]


No, the problem seems to be limited to eating what, on the Unix command line, we'd call operands and option arguments, but which can be URLs with escape sequences like \: and \c in them, and spitting them verbatim into suffixes on roff identifiers, and that just doesn't work in general.


423 .   \"
424 .   \" Handle the case where subcommand is specified as "-class",
425 .   \" setting up appropriate macro aliases for subcommand handlers.
426 .   \"
427 .\" XXX
428 .      if dpdf*href\\$1       .als pdf*href      pdf*href\\$1
429 .      if dpdf*href\\$1.link  .als pdf*href.link pdf*href\\$1.link
430 .      if dpdf*href\\$1.file  .als pdf*href.file pdf*href\\$1.file
431 .   \"
432 .   \" Repeat macro alias setup
433 .   \" for the case where the subcommand is specified as "class",
434 .   \" (without a leading hyphen)
435 .   \"
436 .\" XXX
437 .      if dpdf*href-\\$1      .als pdf*href      pdf*href-\\$1
438 .      if dpdf*href-\\$1.link .als pdf*href.link pdf*href-\\$1.link
439 .      if dpdf*href-\\$1.file .als pdf*href.file pdf*href-\\$1.file


An immense amount of code in pdf.tmac seems to be dedicated to an exploration of the question "hey, what if we chucked established roff programming idioms out the window and re-implemented getopt_long(3) in it so that shell script programmers had macro interfaces that looked vaguely familiar"?

G. Branden Robinson <gbranden>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55114:  gropdf.1.pdf added by deri (218KiB - application/pdf)
file #55090:  t-new.pdf added by deri (68KiB - application/pdf)
file #55085:  t.trf added by deri (3KiB - application/octet-stream)
file #55086:  t.pdf added by deri (68KiB - application/pdf)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by keithmarshall (Posted a comment)
  • -email is unavailable- added by deri (Posted a comment)
  • -email is unavailable- added by gbranden (Submitted the item)
  •  

    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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-09-05 deri Attached File- Added gropdf.1.pdf, #55114
    2023-08-27 deri Attached File- Added t-new.pdf, #55090
    2023-08-26 deri Attached File- Added t.trf, #55085
        Attached File- Added t.pdf, #55086
    2023-08-22 gbranden StatusIn Progress None

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code