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.

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

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.

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

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.

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

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.

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.

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.

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?

Raised for discussion on groff@.

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.

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.