bugGNU roff - Bugs: bug #55334, [preconv] should not blindly use...

 
 

bug #55334: [preconv] should not blindly use libuchardet on an unseekable stream

Submitter:  Eli Zaretskii <eliz>
Submitted:  Sun 30 Dec 2018 03:44:25 PM UTC
   
 
Category:  Preprocessor preconv Severity:  3 - Normal
Item Group:  Incorrect behaviour Status:  Fixed
Privacy:  Public Assigned to:  bgarrigues
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 02 Mar 2022 07:59:29 PM UTC, comment #22: 

comment #21:

> I'll open a new issue for the seekable stream detection issue


Bug #62131

Dave <barx>
Group Member
Wed 02 Mar 2022 07:24:17 PM UTC, comment #21: 

comment #14:

> As a side note, if I run preconv with no arguments, I have to enter CTRL-D on a line by itself twice before it will exit.  Is that a bug?


According to a git bisection...

This turns out to be the same issue as in the OP, or more precisely, Bertrand's fix of 18 October 2020 resolved it as well.

I'm thinking this is due to nested reads from the same stream; preconv does it, and then uchardet does it, but because the stream isn't seekable, you need two EOFs to get out.

I'm therefore going to regard this issue as fixed.

I'll open a new issue for the seekable stream detection issue; I have a patch pending for it.

Bug #59291, also spawned off of this ticket, requests that uchardet be run even on unseekable streams, and that's a heavier lift.  In the meantime, tools like sponge(1) from Joey Hess's "moreutils" project are available, at least on some systems.

Resolving and retitling.


commit c30821d767974c0a5e7220c0e5a65dc963785dc5
Author: Bertrand Garrigues <bertrand.garrigues@laposte.net>
Date:   Sun Oct 18 00:53:32 2020 +0200

    preconv: don't use libuchardet if input is stdin

    * src/preproc/preconv/preconv.cpp (do_file): don't call
      detect_file_encoding if input file is "-"

    This fixes the failure on MS-Windows described #55334, however this
    does not fix the encoding detection with uchardet if the input is
    stdin (the user would have to pass with -D the correct encoding as
    explained in the man page).

-verbatim-

G. Branden Robinson <gbranden>
Group administrator
Wed 02 Mar 2022 05:58:47 PM UTC, comment #20: 

Self-reply after 16 months or so.

comment #14:

> I second Ingo on this.
>
> '-' was always a hack.
>
> As I understand it, it was developed in the days before there were such things as /dev/stdin and /dev/stdout.  A way was needed for some programs to be told via the shell to read or write file descriptors 0 or 1 and the shell had no semantics for this (for redirections, yes--to denote a command operand as a request to re-use fds 0 or 1 instead of opening a new file, no).
>
> Even if /dev/std{in,out} had existed, I get the feeling Thompson wouldn't have used them because that would have been too much typing. 
>
> '-' has no standard system-wide semantics, only a set of command-specific conventions.  Many groff programs replicate those conventions for the sake of AT&T compatibility.  But we're not required to extend that pattern to preconv and I don't think we should.


There's more cement poured over the semantics of "-" as a file name in groff than I knew about back in 2020.  groff(1), if given it as an argument, passes it as-is to all of the preprocessors it calls, and all the traditional preprocessors recognize it.  It doesn't seem like a good idea to me to add complexity to groff(1) to call preconv(1) differently.

> Strictly speaking, the seekability of a file stream should be independent of whether it's one of the "standard" ones or not.  So if I understand the applied patch correctly, this would cause a spurious defeat of uchardet usage:
>


> $ preconv - < foobar


>
> The above is uneconomical, like the famous "useless uses of cat" frequently contrived by Unix novices, but it shouldn't force any conclusions to be drawn about the seekability of standard input.
>
> By contrast,
>


> $ cat foobar | preconv
> $ cat foobar | preconv /dev/stdin


>
> ...both would be ways to render preconv's input un-seekable.


This part, after learning about "-" the hard way, I am working on.

> As a side note, if I run preconv with no arguments, I have to enter CTRL-D on a line by itself twice before it will exit.  Is that a bug?


I haven't explored this yet.

> I also note, more to myself as the man page guy, that preconv(1) doesn't explicitly say what it does if it is given no file arguments, and it should.


I hadn't documented that yet because I had hopes we'd be able to get by without the special "-" file name (which Bertrand already implemented post-1.22.4), but I don't think we can, without excessive complexity cost.

G. Branden Robinson <gbranden>
Group administrator
Fri 04 Jun 2021 10:04:11 AM UTC, comment #19: 

comment #12:

> I'm closing this one and opening a new bug to find a definitive solution.


For the record, that new bug is bug #59291.

Dave <barx>
Group Member
Mon 15 Mar 2021 05:06:46 AM UTC, comment #18: 

The original problem, which Ingo gave a severity of 5, was fixed in the commit mentioned in comment #12.  The issue that prompted the bug's reopening Ingo described as "a minor concern" in comment #13.  So a severity of 5 no longer seems appropriate for the remaining changes needed.  Lowering to 3.

Dave <barx>
Group Member
Fri 13 Nov 2020 11:56:53 AM UTC, comment #17: 

Setting this bug back to Open to reflect the Status being set back to "In Progress."

Dave <barx>
Group Member
Sat 24 Oct 2020 08:51:32 PM UTC, comment #16: 

I will work on it, but I'll first make the 1.23.0.rc1

Bertrand Garrigues <bgarrigues>
Group administrator
Thu 22 Oct 2020 05:10:55 PM UTC, comment #15: 

Re comment #14:  All makes sense to me.

Branden, do you want to write the required patch to avoid guessing from the filename and instead use fseek(3) and check for errors, as i suggested?  And test it?  I don't really want to work on it because i never install uchardet on any of my systems.

Ingo Schwarze <schwarze>
Group Member
Sun 18 Oct 2020 03:23:17 PM UTC, comment #14: 

I second Ingo on this.

'-' was always a hack.

As I understand it, it was developed in the days before there were such things as /dev/stdin and /dev/stdout.  A way was needed for some programs to be told via the shell to read or write file descriptors 0 or 1 and the shell had no semantics for this (for redirections, yes--to denote a command operand as a request to re-use fds 0 or 1 instead of opening a new file, no).

Even if /dev/std{in,out} had existed, I get the feeling Thompson wouldn't have used them because that would have been too much typing. 

'-' has no standard system-wide semantics, only a set of command-specific conventions.  Many groff programs replicate those conventions for the sake of AT&T compatibility.  But we're not required to extend that pattern to preconv and I don't think we should.

Strictly speaking, the seekability of a file stream should be independent of whether it's one of the "standard" ones or not.  So if I understand the applied patch correctly, this would cause a spurious defeat of uchardet usage:


$ preconv - < foobar


The above is uneconomical, like the famous "useless uses of cat" frequently contrived by Unix novices, but it shouldn't force any conclusions to be drawn about the seekability of standard input.

By contrast,


$ cat foobar | preconv
$ cat foobar | preconv /dev/stdin


...both would be ways to render preconv's input un-seekable.

As a side note, if I run preconv with no arguments, I have to enter CTRL-D on a line by itself twice before it will exit.  Is that a bug?

I also note, more to myself as the man page guy, that preconv(1) doesn't explicitly say what it does if it is given no file arguments, and it should.

Corrections welcome on any of the above.

G. Branden Robinson <gbranden>
Group administrator
Sun 18 Oct 2020 01:37:31 PM UTC, comment #13: 

Thanks for fixing this!

I would like to raise a minor concern, maybe you can tweak your solution.  Comparing the filename to "-" seems fragile.  There may be other ways to get a non-seekable file descriptor, for example someone passing "/dev/stdin" on the command line or someone passing the name of a file created with mkfifo(1).

Instead of strcmp(filename, "-") in do_file(), i suggest doing something like the following inside detect_file_encoding():

-rewind(fp);
+if (fseek(fp, 0L, SEEK_SET) == -1)
+    goto end;

That seems like the canonical way to test for the feature actually needed, which feels much more robust than some crude guesswork based on tangentially related string data.  Testing at the place where the feature is really needed (and where other error handling is already present) also seems easier to understand than having an additional check one level up in the call stack.

What do you think?

Ingo Schwarze <schwarze>
Group Member
Sat 17 Oct 2020 11:05:37 PM UTC, comment #12: 

I've pushed (c30821d767974c0a5e7220c0e5a65dc963785dc5) a little fix to not use uchardet if the input is stdin.  I believe this will fix the immediate failure originally described in this bug, so I'm closing this one and opening a new bug to find a definitive solution.

Bertrand Garrigues <bgarrigues>
Group administrator
Thu 10 Jan 2019 09:13:45 PM UTC, comment #11: 

I was thinking of the final call to groff done by pdfmom:

  groff -Tpdf -mom $preconv - $cmdstring"

it passes the standard input from the previous calls (from what I understand, to generate the table of contents) plus the input file; however this does not seem to create any problem ('mon_premier_doc.mom, which has words with French characters in its table of contents, is correctly generated).

Bertrand Garrigues <bgarrigues>
Group administrator
Mon 31 Dec 2018 06:53:59 PM UTC, comment #10: 

If pdfmom is used in a pipeline, or given a filename of "-", it uses a temporary file to store the contents of stdin, so it is unlikely to cause a problem for preconv.

Deri James <deri>
Group Member
Mon 31 Dec 2018 06:33:26 PM UTC, comment #9: 

It was not my intention to give up the correct handling of stdin, but I'd like first to commit the easy fix I mentioned so that -me examples are correctly rendered on Windows.  Then we can open another bug to think about a proper fix on stdin. I would also like to demonstrate the problem on a mom example; as 'pdfmom' makes successive calls to groff (for pdf references) it should be possible to bump into the problem.

Bertrand Garrigues <bgarrigues>
Group administrator
Mon 31 Dec 2018 06:00:06 PM UTC, comment #8: 

Sorry for missing your point.

As for the solution: I would suggest to explore better ways of saving stdin on a temporary file, before concluding we have no other way than punt.  For example, how about using tmpfile?  It should be portable, and is supposed to prevent race conditions, AFAIU.

Eli Zaretskii <eliz>
Mon 31 Dec 2018 05:44:05 PM UTC, comment #7: 

My comment was probably not clear but when I wrote "But of course if 'groff' takes its own input from stdin 'preconv' will also have its input from stdin. [...]The difference is that (in 'doc/doc.am') the input .me  is piped into groff"  I was referring to the command line you mentioned and to the fact that (for .me examples) the build was passing the input of 'groff' through a pipe.

I'll fix the usage of 'detect_file_encoding' (as its names suggests it is meant to be used on files), and we'll see later if we can improve 'preconv' to detect the encoding of the input when it comes from stdin.

Bertrand Garrigues <bgarrigues>
Group administrator
Mon 31 Dec 2018 05:09:39 PM UTC, comment #6: 


> groff' always runs 'preconv' before any other pre-processors, and he always pass the input file as an argument to preconv


That's not always true, and in particular the way the Groff build produces meintro.ps etc. is this:

$ LANG=C  LC_ALL=C  sed -e "s;[@]VERSION[@];1.22.4;" doc/meintro.me | GROFF_COMMAND_PREFIX=  GROFF_BIN_PATH=".../groff-1.22.4"  .../groff-1.22.4/groff -I.../groff-1.22.4 -M .../groff-1.22.4/doc  -M.../groff-1.22.4/tmac -M.../groff-1.22.4/tmac -F.../groff-1.22.4/font -Upet -ww  -k -Tps -ge -me -V

(where I replaced with the ellipsis the absolute file name of the directory where I built Groff).

IOW, the input is passed from Sed via a pipe in this case.

Eli Zaretskii <eliz>
Mon 31 Dec 2018 04:43:31 PM UTC, comment #5: 

Whoa, the code in GNU make-4.2.1/output.c is also somewhat broken.  It does correctly use open(O_CREAT|O_EXCL), but it only tries once (except for EINTR retries), which is vulnerable to denial of service.  Even worse, lacking fdopen(3), it falls back to totally insecure behaviour, rather than using fopen("wx") or erroring out.  I'd call that "prior negligence" rather than "prior art".  But that's kind of off-topic here, we should not adopt insecure idioms here merely because others got it wrong.

You may be right that reading a fixed amount of bytes may not be good enough - you have to keep reading until uchardet can decide, and of course keep the data read in memory such that you can reuse it for the actual processing.  I wouldn't worry too much about memory usage.  Text documents - even whole books - are typically small compared to stuff like videos, so reading lots of text is unlikely to cause memory exhaustion in this century.  Besides, the cues needed by uchardet will hardly be Gigabytes away from the beginning of the file; they will likely be in the first few Kiloybytes, or at worst in the first few Megabytes.

My plea to not commit was of course intended for other groff developers; there is always a risk that someone tests it, finds that it work (which it will, in some cases), and goes ahead to commit, in particular when a patch comes from a well-known free software developer.

Ingo Schwarze <schwarze>
Group Member
Mon 31 Dec 2018 04:28:06 PM UTC, comment #4: 

'groff' always runs 'preconv' before any other pre-processors, and he always pass the input file as an argument to preconv, e.g:

  groff -k foo

triggers:

  preconv foo | troff -Tps | grops

But of course if 'groff' takes its own input from stdin 'preconv' will also have its input from stdin.

In your original report you said that doc/meintro.me and doc/merefer.me where not correctly generated. The difference is that (in 'doc/doc.am') the input .me file is piped into groff (unlike for example mom examples where the input file is passed as an argument).  On GNU/Linux the .me files are still correctly generated because fstat gives a 0 length.

uchardet is needed otherwise 'preconv' defaults to latin1 and non-ascii characters will not be correctly rendered (bug #51330).

An easy solution is not to call 'detect_file_encoding' if the input is "-": uchardet will still be used on files, and if the user pipes something into 'groff' he can still use -D to set the correct encoding.

I will work on it and update the doc.

Bertrand Garrigues <bgarrigues>
Group administrator
Mon 31 Dec 2018 03:57:02 PM UTC, comment #3: 

Btw, there's "prior art" to doing this kind of thing: that's what GNU Make does when invoked with a "-f -" command-line option.

Eli Zaretskii <eliz>
Mon 31 Dec 2018 03:32:05 PM UTC, comment #2: 

Relax, I cannot commit anything anyway, since I don't have write access to the Groff tree.  And I'm all for finding a better way of doing this if it exists.

Your proposal to read just part of the input is not necessarily a better solution, IMO: the telltale part of the input which enables encoding detection could be after the part you decide to read, and input can be very long, thus reading all of it until EOF might not be practical, and will delay the production of output.  This could work with a small number of encodings, such as UTF-16 and EBCDIC.


Eli Zaretskii <eliz>
Mon 31 Dec 2018 01:05:18 PM UTC, comment #1: 

I confirm that this is a bug.  Of course we cannot use fseek(3) on the input stream, and using fstat(2) is also dubious at best.  Since groff utilities are typically used in pipes, this breaks typical use cases, so i'm changing the severity to "blocker"
and the planned release to 1.22.5.

Assigning to bgarrigues@ because he intregrated uchardet.
Of course, anyone is welcome to fix it.

However, i strongly object to the attached patch.  Please do not commit it.  It introduces obvious security vulnerabilities.  In particular, it uses tmpnam(3), which is almost never acceptable and almost always vulnerable, even when used carefully; on top of that, the present patch is using it in a totally naive and careless way.  See https://man.openbsd.org/tmpnam.3#BUGS for details.

Even using mkstemp(3) instead would be very ugly, and i would not particularly like that.  We really shouldn't have to create a temporary file merely for charset recognition.

One possibility for a proper solution might be to read a sufficient amount of text from the input stream into memory, then work purely in memory (for example, mandoc does it that way), but i did not check whether that easily works with the architecture of preconv(1) and uchardet.

If it turns out there is no viable solution, then i would strongly prefer completely removing the broken uchardet support rather than making matters even worse with buggy interfaces like tmpnam(3).

Ingo Schwarze <schwarze>
Group Member
Sun 30 Dec 2018 03:44:25 PM UTC, original submission:  

If preconv is built with libuchardet, it produces partial output, "swallowing" the initial portion of its input.

The reason for that is that the function detect_file_encoding calls 'fstat' and 'fseek' on the input stream, and those are non-portable when the input stream is not a file, but a pipe or a console device.  On MS-Windows, 'fstat' reports a small value (the size of the pipe buffer), and 'fseek' simply fails.  So the initial portion of the inout, read by detect_file_encoding, never shows in the output.

The solution I propose is to copy stdin to a temporary file, and read from that file instead.  The proposed patch is in the attachment.  The changes are currently for MS-Windows only, but maybe other platforms will need them as well.

Eli Zaretskii <eliz>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45792:  preconv_decode.dif added by eliz (2KiB - application/vnd.ms-excel)

 

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 gbranden (Posted a comment)
  • -email is unavailable- added by deri (Posted a comment)
  • -email is unavailable- added by bgarrigues (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by eliz (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-03-02 gbranden StatusIn Progress Fixed
        Assigned togbranden bgarrigues
        Open/ClosedOpen Closed
        Summarypreconv fails when built with libuchardet on MS-Windows [preconv] should not blindly use libuchardet on an unseekable stream
    2022-03-02 gbranden Assigned tobgarrigues gbranden
        Summarypreconv fails when build with libuchardet on MS-Windows preconv fails when built with libuchardet on MS-Windows
    2021-03-15 barx Severity5 - Blocker 3 - Normal
    2020-11-13 barx Open/ClosedClosed Open
    2020-10-24 bgarrigues StatusFixed In Progress
    2020-10-17 bgarrigues Open/ClosedOpen Closed
    2020-10-17 bgarrigues StatusConfirmed Fixed
    2018-12-31 schwarze Severity3 - Normal 5 - Blocker
        StatusNone Confirmed
        Assigned toNone bgarrigues
        Planned ReleaseNone 1.23.0
    2018-12-30 eliz Attached File- Added preconv_decode.dif, #45792

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code