bugGNU roff - Bugs: bug #61423, [libgroff] document change in...

 
 

bug #61423: [libgroff] document change in handling of font "name" directives

Submitter:  Dave <barx>
Submitted:  Thu 04 Nov 2021 04:48:31 AM UTC
   
 
Category:  Core Severity:  4 - Important
Item Group:  Documentation 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

Mon 21 Aug 2023 11:16:46 PM UTC, comment #25: 

comment #9:

> A lot of this comment will need to be copied to a Savannah ticket,


Bug #64579, at your service.

If other things in this epic comment need to be separate tickets, you'll have to help me tease out what they are.

Dave <barx>
Group Member
Mon 24 Oct 2022 06:28:15 AM UTC, comment #24: 

Ah, yes, nearly two months ago--guess I overlooked that.  Thanks!

Dave <barx>
Group Member
Sun 23 Oct 2022 07:57:13 PM UTC, comment #23: 


comment #22:

> comment #17:
> > This change could also be highlighted in the next rc
> > announcement as one of the items used to whip up interest in
> > testing, since it may break some users' existing font files.
>
> Does this announcement exist in draft form somewhere, or at least as a list of things it should include?  If not, should I open a savannah ticket to gather such a list?


Hi Dave,

Yes, I created the "ANNOUNCE" file in the source tree for this purpose.

https://git.savannah.gnu.org/cgit/groff.git/tree/ANNOUNCE

G. Branden Robinson <gbranden>
Group administrator
Sun 23 Oct 2022 07:48:29 PM UTC, comment #22: 

comment #17:

> This change could also be highlighted in the next rc
> announcement as one of the items used to whip up interest in
> testing, since it may break some users' existing font files.


Does this announcement exist in draft form somewhere, or at least as a list of things it should include?  If not, should I open a savannah ticket to gather such a list?

(I presume the rc announcement would largely include the same info that will ultimately go into the release announcement.  The email thread about the latter announcement (http://lists.gnu.org/r/groff/2022-08/msg00215.html) forms the start of a list, but adding to the thread at this point won't link to the rest of the thread in the archive because of the archiving software's month-boundary limitation.)

Dave <barx>
Group Member
Thu 06 Oct 2022 01:26:31 PM UTC, comment #21: 


commit fb948f55c00f8d472d61f0c94c0cd7c7ffd9c390
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Oct 4 05:52:55 2022 -0500

    NEWS: Add items.

    * NEWS: Advise users of changes to "name" directive handling in font
      description files.

    Fixes <https://savannah.gnu.org/bugs/?61423>.  Thanks to Dave Kemper for
    the report.


G. Branden Robinson <gbranden>
Group administrator
Tue 04 Oct 2022 08:32:23 AM UTC, comment #20: 

Proposed text.


diff --git a/NEWS b/NEWS
index a28db5864..2a5f15f56 100644
--- a/NEWS
+++ b/NEWS
@@ -74,6 +74,11 @@ o New requests `stringdown` and `stringup` are available.  These change
 o The `ab` request no longer writes "User Abort." to the standard error
   stream if given no arguments.

+o The `fp` request no longer accepts file or font names with slashes in
+  them as arguments.  All font description files are expected to be
+  accessible within the directory of the output device for which they
+  were prepared.
+
 nroff
 -----

@@ -580,6 +585,11 @@ o The 'doc' Make target has been eliminated.  'all' (the default Make
   manual's sources are modified; the distribution archive now provides
   copies of the manual in Info, plain text, HTML, DVI, and PDF.

+o The "afmtodit" utility no longer writes file names with directory
+  information in them to the "name" directives of the font descriptions
+  it generates.  (The `fp` request no longer accepts such names; see
+  "troff" above.)
+
 o groffer has been deleted from the distribution.

 o grog no longer supports the "--warnings" option; the one diagnostic


G. Branden Robinson <gbranden>
Group administrator
Fri 03 Jun 2022 05:20:46 AM UTC, comment #19: 

I'd like to note the following recent commit, which should make it really obvious when shenanigans like my "bogusfont" example are attempted.

https://git.savannah.gnu.org/cgit/groff.git/commit/?id=95bff60198eafc9f0ee326ad9f961b0528ec9610

G. Branden Robinson <gbranden>
Group administrator
Fri 03 Jun 2022 02:38:52 AM UTC, comment #18: 

comment #17:

> One question of comment #14--whether the tightening of the scope of the font file's "name" directive should appear in NEWS--is also pending.
>
> This change could also be highlighted in the next rc announcement as one of the items used to whip up interest in testing, since it may break some users' existing font files.


Setting this issue to "Blocker" so that I don't forget to communicate adequately with the users about this, or justify here why I think it doesn't need such a prominent notice.

G. Branden Robinson <gbranden>
Group administrator
Wed 16 Mar 2022 10:18:47 PM UTC, comment #17: 

I'm willing to retract the current summary based on subsequent discussion.  Comment #13 codifies the decision made of the options outlined in comment #10, and I'm on board with that choice.

There still seem to be a design decision and some specific actions to be taken based on the deep dive of comment #9, but perhaps the simplest path forward is to close this bug and open (a) new one(s) based on those items.

One question of comment #14--whether the tightening of the scope of the font file's "name" directive should appear in NEWS--is also pending.

This change could also be highlighted in the next rc announcement as one of the items used to whip up interest in testing, since it may break some users' existing font files.

Dave <barx>
Group Member
Tue 15 Mar 2022 08:42:53 AM UTC, comment #16: 

Setting severity to "wish" as the advisability of the change as currently summarized ("allow paths in "name" directive of font description file, restoring historical groff behavior") is not yet determined.

At present I lean against it, but we haven't done a groff release that rejects such things (as Git HEAD does) so the desire for such a feature among our user base, and the breadth of existing reliance upon it, has not been established.

G. Branden Robinson <gbranden>
Group administrator
Tue 08 Mar 2022 05:51:22 AM UTC, comment #15: 

comment #12:

> So if groff now disallows a path here, afmtodit ought to at
> least squawk if the user asks for one, if not omit it entirely.


Bug #62150

Dave <barx>
Group Member
Wed 29 Dec 2021 06:20:46 PM UTC, comment #14: 

comment #13:

> Would you agree that the present behavior of groff in Git
> conforms to your option B?


Yes.  And, if I'm understanding correctly, also to your option 1.

> No one has raised Cain about this (or even uttered a peep in
> protest), so I reckon I will leave it until it draws complaint.


OK.  So the remaining questions are:

  • Is this a NEWS-worthy change?
  • Does this warrant a bug report against afmtodit?


Comment #9 seems to call for at least two other tickets, in its "In the short term" and "In the longer term" paragraphs.

Dave <barx>
Group Member
Tue 28 Dec 2021 06:10:37 AM UTC, comment #13: 

comment #12:

> comment #11:
> > if we do, it suggests another bug report needs to be filed
> > against whatever tool is putting full paths there.
>
> The (good? bad?) news is that this tool is part of groff: afmtodit.
>
> All my font description files produced by install-font.sh have a comment at the top saying "This file has been generated with GNU afmtodit" (followed by version numbers ranging from 1.20.1 to 1.22.3) and a "name" directive containing only the basename.  So install-font.sh is calling afmtodit in a way that does not put a full path there.
>
> The problematic font description file also has the comment crediting afmtodit (specifically version 1.22.2), but this file I generated using a script Tadziu posted (http://lists.gnu.org/r/groff/2013-11/msg00042.html).
>
> Thus, afmtodit does not routinely include paths in the "name" directive, but can be coaxed into doing so.  So if groff now disallows a path here, afmtodit ought to at least squawk if the user asks for one, if not omit it entirely.
>
> Whether this warrants a separate bug report will depend, I think, on what decisions are made of the choices offered in comment #10 and comment #9.


Hi, Dave!

Would you agree that the present behavior of groff in Git
conforms to your option B?

When I give it the reproducer from bug #61424, I get the
following output.


$ ./build/test-groff -Tutf8 EXPERIMENTS/hello-dave.roff
troff:EXPERIMENTS/hello-dave.roff:4: error: cannot load font '../../../../../../../../../../../home/branden/bogusfont' for mounting
troff:EXPERIMENTS/hello-dave.roff:5: warning: no font mounted at position 5
WORD


No one has raised Cain about this (or even uttered a peep in
protest), so I reckon I will leave it until it draws complaint.

G. Branden Robinson <gbranden>
Group administrator
Fri 24 Dec 2021 01:35:20 AM UTC, comment #12: 

comment #11:

> if we do, it suggests another bug report needs to be filed
> against whatever tool is putting full paths there.


The (good? bad?) news is that this tool is part of groff: afmtodit.

All my font description files produced by install-font.sh have a comment at the top saying "This file has been generated with GNU afmtodit" (followed by version numbers ranging from 1.20.1 to 1.22.3) and a "name" directive containing only the basename.  So install-font.sh is calling afmtodit in a way that does not put a full path there.

The problematic font description file also has the comment crediting afmtodit (specifically version 1.22.2), but this file I generated using a script Tadziu posted (http://lists.gnu.org/r/groff/2013-11/msg00042.html).

Thus, afmtodit does not routinely include paths in the "name" directive, but can be coaxed into doing so.  So if groff now disallows a path here, afmtodit ought to at least squawk if the user asks for one, if not omit it entirely.

Whether this warrants a separate bug report will depend, I think, on what decisions are made of the choices offered in comment #10 and comment #9.

Dave <barx>
Group Member
Fri 19 Nov 2021 09:28:41 PM UTC, comment #11: 

comment #9:

> I think option #2 would be more in line with what Dave was already doing.


I wasn't doing anything at all with that directive line.  I wasn't supplying a path to any parameter of my .fp calls; the files in question all live in a place groff looks by default (/usr/share/groff/site-font).

But it's worth noting that I also didn't handcraft the font description file in question.  Some toolchain did it -- offhand, I don't know what (though I could probably track it down), except that it's not Peter's install-font.sh, because that's my usual tool, and it's generated a bunch of other font description files that don't have paths in the "name" directive -- and the toolchain is not one of my invention, so other people using the same tool(s) will have ended up with paths in their "name" directives too.

That's not a solid argument against disallowing such paths, if we deem that the best thing for groff; but if we do, it suggests another bug report needs to be filed against whatever tool is putting full paths there.

Dave <barx>
Group Member
Mon 08 Nov 2021 11:54:13 PM UTC, comment #10: 

comment #9:

> The scope of this ticket remains deciding between
> the 2 options above in `fp` request handling.
>
> Whew.  I think I'm going to pour myself a drink.


I poured myself one and then lost the ability to follow 97% of that explanation.

However, it seems to me the scope of this ticket is even more modest than what's proposed above.  The resolution of bug #61424 seems to have closed the security hole.  As long as .fp is disallowed from including pathnames, the pathname part (if present) of the font description file's "name" directive can (maybe?) be safely ignored, and only the basename portion compared against...whatever it ends up getting compared against based on the choice of the aforementioned option 1 or 2.

So I think for this bug, the options are (I'll letter them so as not to confuse them with the preexisting options 1 and 2):

A. Ignore the dirname part (if any) of the "name" directive;
B. Disallow a dirname within the "name" directive and (as is presently done) treat the entire field as the font name to be matched against; or
C. Split the difference, keeping the current behavior but downgrading the problem from an error to a warning, allowing the file to be processed but letting the user know that that file no longer comports with the current font description file spec.

Option B would mildly break back compatibility, but the present wording of the error makes it easy for the user to zero in on the problem and fix it.  Option A runs into the problem elucidated in comment #1 of different OSes using different directory separators.

If there are other issues at play, they're lost in my fuzzy understanding of the overall situation.

Dave <barx>
Group Member
Sun 07 Nov 2021 07:20:18 AM UTC, comment #9: 

This ticket appears to have turned over a very old rock with some very ugly creatures living beneath it.

It appears that the argument to the `name` directive isn't necessarily a file name.

It's an "external name".  Both CSTR #54 and existing groff documentation are pretty vague about what that means.

CSTR #54, on the `fp` request:

> This is a statement that a font named F is associated with position N. It is a fatal error if F is not known. For fonts with names longer than two characters, L refers to the long name, and F becomes a synonym.


(I note as an aside that groff does not treat the foregoing failure mode as a fatal error, nor in my opinion should it.)

Regarding the font description file itself, CSTR #54 is hopelessly circular.

> name str <tab> name of font is str


Wow, thanks.

groff's own Texinfo manual says this:


The 'fp' request has an optional third argument.  This argument
gives the external name of the font, which is used for finding the
font description file.  The second argument gives the internal name
of the font, which is used to refer to the font in 'gtroff' after
it has been mounted.  If there is no third argument then the
internal name is used as the external name.  This feature makes it
possible to use fonts with long names in compatibility mode.


If the above were completely true, then there would be no reason for font description files to have an `internalname` directive; a font's internal name would be completely document-dependent.

Indeed, I can say:


.fp 5 Normal R
.fp 6 Slanted I
.fp 7 Heavy B
.fp 8 Emphatic BI
.ft Normal
Hello, Joe.
What
.ft Slanted
do
.ft Heavy
you
.ft Emphatic
know?


And it works as you would expect in groff 1.22.4 and Git HEAD.  (Hooray, something I didn't regress.)

But if a font's internal name is document-dependent, what are we to make of `internalname` as used (only) by grodvi, grops, and grotty?

It seems like we're using the highly specific jargon "internal[ ]name" to mean two completely different things.  One is a within-document alias for a font name (or for a font mounting position, for that matter, which is what a font name already is); the other is a driver-specific tag used only for communications between groff objects of the class `font` and the output drivers.

I note that the member function `font::get_internal_name`, which refers only to this latter concept, has no call sites outside of specific output drivers.


$ git grep get_internal_name
src/devices/grodvi/dvi.cpp:  const char *nm = f->get_internal_name();
src/devices/grops/ps.cpp:     .put_literal_symbol(f->get_internal_name())
src/devices/grops/ps.cpp:  const char *psname = sty.f->get_internal_name();
src/devices/grops/ps.cpp:    rm.need_font(psf->get_internal_name());
src/devices/grops/ps.cpp:       .put_literal_symbol(tem->p->get_internal_name())
src/devices/grotty/tty.cpp:  const char *num = f->get_internal_name();
src/include/font.h:  const char *get_internal_name();   // Return the 'internalname'
src/include/font.h:                     // a null pointer.  Used by get_internal_name().
src/libs/libgroff/font.cpp:const char *font::get_internal_name()


This member function accesses a member variable called `internalname`.  These member names are very confusing.  They have to do only with the device-specific internal name.  The `internalname` member variable is never read by any code elsewhere in libgroff; its accessor member function is called only by output drivers.  For grodvi, it's supposed to refer to the name of the corresponding TFM file.  For grotty, it's a bit mask, for Christ's sake.

I think we have a choice ahead of us:

1. Validate the font description file's `name` directive upon attempted load against the "internal name" (note the space), the mandatory 2nd argument to `fp`.  If we do this, slashes are indeed prohibited.  it will also serve to verify that this internal name is obviously related to the name of the font description file on the file system.

This "internal name" (mind the gap) is what is referred to as `name` in the `font` class, and it is what is ultimately passed to `font::open_file` in fontfile.cpp.

(One might reasonably ask why this `name` (really, `nm`) is being passed to `open_file` when `name` is a member variable of an object of the very class whose member function this is.  Why do we pass it when we should already know it?  The answer is that an object of class `font` is not completely initialized until the font description file has been read, and that hasn't happened yet before the file has been opened.  In fact, there's a lot of validation that has to happen before a font object can be assumed valid.  It is my recent addition of more such validation that prompted Dave to file this ticket.)

But maybe no one actually needs validation of this sort.  If they `cp A B` inside a font directory, maybe we shouldn't stop them from loading the font description file `B` without changes.

2. Another possibility is to validate the value of the `name` directive against the external name supplied to the `fp` request.  Since that external name is an optional argument, the validation would only be performed if it were present.  Assuming that a disciplined name space grew up around external font names in the groff ecosystem, documents and macro packages could then use the `fp` request to ensure that they're getting exactly the fonts they wanted.  (A discipline of syncing this external name and `name` directive contents with actual font files, like version x.y.z of font FooBar from Foundry BigCorp, is a necessary but independent prerequisite for this benefit to be realized.)

One of the things TeX touts about itself is its core principle of never regressing the renderability of anything.  Knuth and his descendants took this principle so seriously that they leveraged the file system as a registry and wrote filename invariance into their software licenses, eventually leading to tension with the Free Software movement's equally deep dedication to the principles of modularity and interchangeability.

The file system was used as a registry because it was guaranteed to be there (in a rare move, Knuth kept his yak-shaving clippers in his pocket).  I disagree with that choice but sympathize with the motives behind it.  groff could do worse than to offer one, for fonts, via this mechanism.  For an example of how not to do a font registry in a way that humans want to deal with, see X (Window System) logical font descriptions.

I think option #2 would be more in line with what Dave was already doing.

Another independent but important issue in my view is to deprecate that driver-specific `internalname` member variable as soon as possible in favor of some other name.  The design of the font description file format suggests that a driver could pick any usefully descriptive name it wanted, however I don't think libgroff offers the font description as a general key/value store.  This `FONT_COMMAND_HANDLER` business appears to be the correct the mechanism, so that output drivers would extend the class and supply their own member functions, and grodvi, grolbp, grolj4, and grops all seem to have done so for other directive names.  But this wasn't done for `internalname`.  For some reason, the `internalname` member variable became an ersatz union--in other words, a junk box.

In the short term I propose to rename the `internalname` and `get_internal_name` members of `font` to something else--almost anything else, even arrant nonsense, like "mxyzptlk", would be an improvement because at least then it would be impossible to infer identity with the other, much higher-profile, thing groff calls an "internal name".

For compatibility with old font files, recognition of the `internalname` directive name would have to be retained.  But we can use the new name going forward.  Right now I don't have a better idea for a new name than "disambiguator" or "classifier", which are pretty weak...I challenge the reader to come up with a term that can encompass grodvi, grops, and grotty's usages (TFM file name, PostScript font name, bit mask).

In the longer term, I guess the `FONT_COMMAND_HANDLER` thing should actually be exercised on the output driver side of things as seems to have been originally intended.  Then the `font` class wouldn't need the `get_internal_name` member function at all.

A lot of this comment will need to be copied to a Savannah ticket, but I wanted to get this down while the result of my investigations was fresh in my head.

The scope of this ticket remains deciding between the 2 options above in `fp` request handling.

Whew.  I think I'm going to pour myself a drink.

G. Branden Robinson <gbranden>
Group administrator
Sat 06 Nov 2021 02:54:00 AM UTC, comment #8: 


comment #7:

> 61324 concerns changing only a tmac file, so you presumably intended 61424 here?


Yes, indeed--thanks, Dave.

And in my email to the list I started out okay but then my brain derailed and I started calling the font description file directive "file" instead of "name".  Argh.

I'm not getting much feedback to my survey, but the wind seems to be blowing against directory traversal and therefore for the sterner validation of the "name" directive I already put in place. 

I can put a sed script into the commit message that will copy pathful "name" directive entries to a comment, leaving only a basename-y one to be validated.

G. Branden Robinson <gbranden>
Group administrator
Fri 05 Nov 2021 09:13:36 PM UTC, comment #7: 

comment #3:

> I can do something unpleasant in groff 1.22.4, so I'll open
> a separate, higher-severity ticket for that.


That's bug #61424, for those following along at home.


comment #4:

> So in addressing bug #61324 I'll probably need to muck with
> the file opening logic for fonts.


61324 concerns changing only a tmac file, so you presumably intended 61424 here?

Dave <barx>
Group Member
Thu 04 Nov 2021 06:42:55 PM UTC, comment #6: 

Yeah, I also hadn't considered pathnames in .fp requests when I opened this change request.  If there's a good security reason to prohibit paths in the "name" directive, editing my handful of nonconforming font files to make them get in line is simple enough.  I just didn't want to see previously working font files rejected on some capricious technicality.

Dave <barx>
Group Member
Thu 04 Nov 2021 03:15:51 PM UTC, comment #5: 
G. Branden Robinson <gbranden>
Group administrator
Thu 04 Nov 2021 10:09:47 AM UTC, comment #4: 

Hmm, interestingly, the change being complained about made an unintended directory traversal pretty unlikely.

So in addressing bug #61324 I'll probably need to muck with the file opening logic for fonts.

It would be easier to characterize the scenario Dave describes as simply unsupported, and invite him to use a meaningless directive name or a '#' comment to store the qualified file name instead.

What do you think, Dave?


$ ./build/test-groff -Tutf8 EXPERIMENTS/the-word-is-evil.roff
troff: /home/branden/src/GIT/groff/build/font/devutf8/../../../../../../../../../home/branden/bogusfont-1.23.0:1: error: font description file name '../../../../../../../../../home/branden/bogusfont-1.23.0' does not match 'name' argument 'bogusfont-1.23.0'
troff:EXPERIMENTS/the-word-is-evil.roff:1: error: cannot load font '../../../../../../../../../home/branden/bogusfont-1.23.0' for mounting
troff:EXPERIMENTS/the-word-is-evil.roff:2: warning: no font mounted at position 5
WORD


G. Branden Robinson <gbranden>
Group administrator
Thu 04 Nov 2021 09:05:09 AM UTC, comment #3: 

Oh, look, there's a directory traversal problem, present since day one.

In testing a fix in progress, I managed to get the "opposite" complaint from the one you had.


$ ./build/test-groff -Tutf8
.fp 5 ./R
troff: /home/branden/src/GIT/groff/build/font/devutf8/./R:1: error: font description file name './R' does not match 'name' argument 'R'
troff: backtrace: file '<standard input>':1
troff:<standard input>:1: error: cannot load font './R' for mounting
.fp 5 ../devutf/R
troff: backtrace: file '<standard input>':2
troff:<standard input>:2: error: cannot load font '../devutf/R' for mounting
.fp 5 ../devutf8/R
troff: /home/branden/src/GIT/groff/build/font/devutf8/../devutf8/R:1: error: font description file name '../devutf8/R' does not match 'name' argument 'R'
troff: backtrace: file '<standard input>':3
troff:<standard input>:3: error: cannot load font '../devutf8/R' for mounting


I can do something unpleasant in groff 1.22.4, so I'll open a separate, higher-severity ticket for that.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 Nov 2021 07:55:13 AM UTC, comment #2: 

The thing I love about making half-informed speculations is how quickly someone thunders to my door to correct me.  Sometimes, I even manage to do it to myself.

comment #1:

> As tempting as it would be to say that this is why we don't validate the value of the `name` directive in the first place, I don't believe that's true.  It was simply never implemented, going all the way back to 1991.  The `if (strcmp(p, "name") == 0))` test was followed by a pair of empty braces.  And in 1991 a port of groff to MS operating systems may well have been regarded as unthinkable: not only was the 8.3 file name convention the only one available on the platform back then, but I can't imagine that the C++ compiler situation was a happy one.  (Back then, the way forward was going to be OS/2, anyway.)


And then there's this from our ChangeLog.115, which goes back to the dawn of recorded groff history.


Sun Sep 29 08:40:57 1991  James Clark  (jjc at jclark)

        * pic/pic.y (YYDEBUG): Don't define for Borland C++.
[...]
        * pic/main.c: On MSDOS munge argv[0].

        * lib/ptable.h: Define name2 as _Paste2 for Borland C++.


So, yes, early groff did support MS-DOS, and as Borland C++ was only ever a product for MS-DOS/Windows, that speaks to the compiler situation.

I still want (partial) validation anyway, because copying one font file to serve as a template for another is the kind of thing a groff device/font hacker is going to do, and it's extremely confusing to have a text file lie to your face about what it is.

Essentially the stuff in the directive argument before the last slash will become a comment field, just as the entire argument has been an (undocumented) comment field since 1991.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 Nov 2021 07:41:46 AM UTC, comment #1: 


original submission:

> My system has a /usr/share/groff/site-font/devps with a hodgepodge of font description files collected over the years through means I don't always remember.  Most of these files contain a simple "name" directive that merely repeats the filename.
>
> A few files contain a full pathname in this field.  These files have worked fine for the past couple of groff releases, all the way up through 1.23.0.rc1.
>
> A recent change -- I'm gonna go out on a limb and accuse September's commit c0d1bb28, though I've not done before/after testing to confirm this -- now prevents these files from loading, troff giving up with the error "font description file name 'JR' does not match 'name' argument '/full/path/to/JR'".
>
> (In my case, the /full/path/to/JR as specified in the font description file did not match the actual full path of its final resting place; apparently I used some tool to generate the file in a working directory, and this tool encoded the full path of its working directory onto this line.  But even when I updated the path to match the file's current location, it still failed with the same error.)
>
> The current Texinfo document says this directive contains "the name of the font," which does suggest that /a/list/of/directories before it might be unexpected.  However, that did work historically.
>
> Editing the font description file to contain what troff wants here is fairly simple.  However, is there any benefit to the user of now disallowing a full path in this field that was previously allowed?
>
> If so, that's fine, and this change request can be happily closed as "Wont Fix".  But if not, it might be better to not gratuitously break font description files that have historically worked.


Hi Dave,

I think you're probably right.  I did not test this scenario.  It is not one I intended to regress.  I wished only to validate the "basename" of the file name, and did not consider the possibility of a `file` directive followed by something with directory components in it.

This does raise the question of Windows file names, where the directory separator is `\` instead of `/`.  Dealing with this is probably a bit harder than just calling `strrchr()` on `p` (the directive's argument) and starting the `strcmp()` from the location it returns, because `\` is an escape character in C literals.

On the other hand, I don't know what {to, I can} expect from an underlying Windows C library.  The Windows ecosystem seems to be fragmenting and resorting to virtualization or WSL, and the last time (or two) I asked for porting help from Windows users, even just to test a build, I heard nothing back.  With a sufficiently faithful POSIX layer, Windows doesn't need different file name handling at all.  And I don't know what the state of play is.  Apparently Windows 7 was the last version to support Interix, which is now dead.

I reckon I can fix this problem for POSIX-like systems only and see, at some point in the future, if someone builds for some Windows-based environment and complains of it.  Maybe then I can get them to test a fix.  I certainly don't want to blindly call into the labyrinthine port (rewrite?) of TeX's kpathsea library we have that deals with these issues.  It's very hard to understand and no one seems to want to touch it.  (The last time Ingo saw it, he wrote a lengthy critique.)

As tempting as it would be to say that this is why we don't validate the value of the `name` directive in the first place, I don't believe that's true.  It was simply never implemented, going all the way back to 1991.  The `if (strcmp(p, "name") == 0))` test was followed by a pair of empty braces.  And in 1991 a port of groff to MS operating systems may well have been regarded as unthinkable: not only was the 8.3 file name convention the only one available on the platform back then, but I can't imagine that the C++ compiler situation was a happy one.  (Back then, the way forward was going to be OS/2, anyway.)

I'll have a look.

G. Branden Robinson <gbranden>
Group administrator
Thu 04 Nov 2021 04:48:31 AM UTC, original submission:  

My system has a /usr/share/groff/site-font/devps with a hodgepodge of font description files collected over the years through means I don't always remember.  Most of these files contain a simple "name" directive that merely repeats the filename.

A few files contain a full pathname in this field.  These files have worked fine for the past couple of groff releases, all the way up through 1.23.0.rc1.

A recent change -- I'm gonna go out on a limb and accuse September's commit c0d1bb28, though I've not done before/after testing to confirm this -- now prevents these files from loading, troff giving up with the error "font description file name 'JR' does not match 'name' argument '/full/path/to/JR'".

(In my case, the /full/path/to/JR as specified in the font description file did not match the actual full path of its final resting place; apparently I used some tool to generate the file in a working directory, and this tool encoded the full path of its working directory onto this line.  But even when I updated the path to match the file's current location, it still failed with the same error.)

The current Texinfo document says this directive contains "the name of the font," which does suggest that /a/list/of/directories before it might be unexpected.  However, that did work historically.

Editing the font description file to contain what troff wants here is fairly simple.  However, is there any benefit to the user of now disallowing a full path in this field that was previously allowed?

If so, that's fine, and this change request can be happily closed as "Wont Fix".  But if not, it might be better to not gratuitously break font description files that have historically worked.

Dave <barx>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by barx (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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-06 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2022-10-04 gbranden CategoryFont devps Core
    2022-10-04 gbranden Summary[libgroff] allow paths in &quot;name&quot; directive of font description file, restoring historical groff behavior [libgroff] document change in handling of font "name" directives
    2022-10-04 gbranden Severity3 - Normal 4 - Important
        Item GroupFeature change Documentation
        StatusNeed Info In Progress
    2022-09-26 gbranden Severity5 - Blocker 3 - Normal
        Planned ReleaseNone 1.23.0
    2022-06-03 gbranden Severity1 - Wish 5 - Blocker
    2022-03-15 gbranden Severity2 - Minor 1 - Wish
    2021-11-04 gbranden StatusIn Progress Need Info
    2021-11-04 gbranden Severity1 - Wish 2 - Minor
        StatusNone In Progress
    2021-11-04 gbranden Assigned toNone gbranden
        Summaryallow paths in &quot;name&quot; directive of font description file, restoring historical groff behavior [libgroff] allow paths in "name" directive of font description file, restoring historical groff behavior

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code