bugGNU troff - Bugs: bug #55334, preconv fails when build with...

 
 

bug #55334: preconv fails when build with libuchardet on MS-Windows

Submitted by:  Eli Zaretskii <eliz>
Submitted on:  Sun 30 Dec 2018 03:44:25 PM UTC  
 
Category:  Preprocessor preconv Severity:  5 - Blocker
Item Group:  Incorrect behaviour Status:  Confirmed
Privacy:  Public Assigned to:  Bertrand Garrigues <bgarrigues>
Open/Closed:  Open Planned Release:  1.22.5

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project 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 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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-31 schwarze Severity3 - Normal => 5 - Blocker
        StatusNone => Confirmed
        Assigned toNone => bgarrigues
        Planned ReleaseNone => 1.22.5
    2018-12-30 eliz Attached File- => Added preconv_decode.dif, #45792

    Back to the top


    Powered by Savane 3.5