groff
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Releasing groff 1.22.5?


From: Ingo Schwarze
Subject: Re: Releasing groff 1.22.5?
Date: Thu, 15 Oct 2020 17:32:32 +0200
User-agent: Mutt/1.12.2 (2019-09-21)

Hi Bertrand,

Bertrand Garrigues via wrote on Thu, Oct 15, 2020 at 12:03:44AM +0200:
> On Mon, Oct 12 2020 at 06:08:27 PM, Ingo Schwarze <schwarze@usta.de> wrote:

>> A major problem with gnulib emerged just now that we should perhaps
>> treat as a blocker.
> [...]
>> The code in
>>
>>   gnulib/lib/vasnprintf.c
>>
>> line 4879 puts a format string containing a %n directive into
>> writeable memory and subsequently passes that memory as a first
>> argument to printf(3).
>>
>> Using %n at all is insecure programming practice.
> [...]

> That seems to be only for old versions of glibc or a few operating
> systems:
> 
>    # if ! (((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3)) \
>             && !defined __UCLIBC__)                                     \
>            || (defined __APPLE__ && defined __MACH__)                   \
>            || (defined _WIN32 && ! defined __CYGWIN__))
>                    fbp[1] = '%';
>                    fbp[2] = 'n';
>                    fbp[3] = '\0';
>    # else

Well, what that code means is "*everywhere* except on newer glibc
and some cases of Apple and Windows" - so it did trigger on OpenBSD.

> From what I understand in their comment after the #else they try do to
> their best to avoid using %n.  You are referring to line 4879, which
> means you were looking on the gnulib integrated on the 1.22.4.  We also
> have the same logic on the up-to-date gnulib (line 5126):
> 
>    /* On systems where we know that snprintf's return value
>       conforms to ISO C 99 (HAVE_SNPRINTF_RETVAL_C99) and that
>       snprintf always produces NUL-terminated strings
>       (HAVE_SNPRINTF_TRUNCATION_C99), it is possible to avoid
>       using %n
>    [...]

Oh, you have a point here.  After your recent gnulib update - thanks
for doing that, even though it caused a minor glitch, see my Savannah
ticket 59276, keeping libraries up to date does make sense - the
condition is now inverted and reads as follows:

  # if ((HAVE_SNPRINTF_RETVAL_C99 && HAVE_SNPRINTF_TRUNCATION_C99)    \
      || ((__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3)) \
          && !defined __UCLIBC__)                                     \
      || (defined __APPLE__ && defined __MACH__)                      \
      || defined __ANDROID__                                          \
      || (defined _WIN32 && ! defined __CYGWIN__))

Even though the nasty OS-tests are still there, they are now obsolete
and instead, %n is only used if HAVE_SNPRINTF_RETVAL_C99
or HAVE_SNPRINTF_TRUNCATION_C99 (or both) fail.

Both succeed on OpenBSD, so i confirmed with the follwoing command
that the insecure code with %n is no longer compiled, for now:

  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I.. -I./src/include
  -I../src/include -I../lib -I./src/include -I./lib -I/usr/local/include
  -g -O2 -c -save-temps -o lib/vasnprintf.o ../lib/vasnprintf.c

(Notice the -save-temps in there.)

Meanwhile, i heard rumours that the upcoming change in libc that will
cause %n in writeable memory to abort the program will cause some
gnulib ./configure tests to (wrongly) fail, again resulting in
compilation of vasnprintf.c with %n and ultimately killing groff
at run-time.  I'll have to check whether such a problem really
exists for groff.  If yes, i'll have to investigate further and
find a fix.  If not, all this can be postponed until after release.

>    #else      /* AIX <= 5.1, HP-UX, IRIX, OSF/1, Solaris <= 9, BeOS */

Uh oh.  Are we really trying to support *those*?
Well, maybe that's a different discussion...

>                    fbp[1] = '%';
>                    fbp[2] = 'n';
>                    fbp[3] = '\0';
>    #endif
> 
> Does your build bump into the code after the #else?

It did until last week, it does not right now, but i have to check
whether it will start doing that again in the near future.
I'll come back to you after checking that.

>> The way gnulib detects whether to compile that code seems to be
>> very broken.  On the one hand, gnulib performs extremely complex
>> tests that resemble an (overly aggressive) regression suite rather
>> than mere feature tests.  If i understand correctly, if *anything*
>> in that suite fails, no matter whether it's important or just a
>> very weird corner case, gnulib tends to compile the file
>> gnulib/lib/vasnprintf.c.

> I think we have vasnprintf.c only because it is a dependency of the
> vsnprintf module (listed in 'gnulib_modules' in bootstrap.conf).

Are you really sure?

Merely for testing (not intended for commit at this point),
i just deleted "vsnprintf" from bootstrap.conf.  While that does
remove "vsnprintf" , "lib/vsnprintf.c", and "m4/vsnprintf.m4"
from bootstrap output, "vasnprintf" is still there as a dependency
of "snprintf", and "lib/vasnprintf.c" and "m4/vasnprintf.m4" are
still in the file list.  Also, vasnprintf.c still gets compiled
for me, presumably because (hard to be sure due to the complexity,
but that's my guess at this point):

  checking whether printf supports the 'a' and 'A' directives... no
  checking whether printf survives out-of-memory conditions... no

Of course, these two results are bogus.  We do support %a/%A, and
i didn't investigate yet what the second one means, but whatever
it may mean, that's clearly not a good reason to replace native
printf(3).

>> On the other hand, that file does lots
>> of very complicated version number and operating system name tests
>> on its own at compile time rather than at ./configure time, so it
>> is very difficult to figure out what is actually happening, and
>> even more so to check whether what is happening in a specific
>> compilation run is correct.
>>
>> For example, it appears that one among large numbers of effects
>> that can trigger compilation of gnulib/lib/vasnprintf.c is that the
>> operating system denies %n in writeable memory, and as a consequence
>> gnulib/lib/vasnprintf.c is compiled and then proceeds to use %n in
>> writeable memory.
> [...]

> From what I understand we build this code only on some old platforms.
> The code that uses %n is not built on my system.

You probably have glibc, right?

> [...]
>> While i did rarely run into operating systems that failed to provide
>> the non-standard function vasprintf(3) - a GNU extension also
>> available in all BSDs - groff does not use that function.  I do not
>> remember ever running into a system lacking any of the standard
>> functions * fprintf(3) - first appeared in Version 7 AT&T UNIX *
>> snprintf(3) and vsnprintf(3) - first in 4.4BSD
>>
>> So my favourite solution would be to just stop using all the gnulib
>> *printf* modules.  I'm not aware of any portability problems they
>> might help to solve, not even on historic systems like Solaris 9,

> the *printf* modules from gnulib were used in replacement of some
> printf functions implemented directly in groff's source code.

That's probably not a good idea either.

> I believe that
> the gnulib's versions are much better maintained.  Perhaps that now
> groff builds fine without any replacement function at all, but that
> would require to spend extra time to check that, so unless it is clearly
> proven that there is a problem in gnulib's replacement functions I don't
> see any reason to remove them.

If i manage to confirm that the tests for HAVE_SNPRINTF_RETVAL_C99
and HAVE_SNPRINTF_TRUNCATION_C99 are reasoably robust, we can
postpone further deliberation until after release, i think.

>> but they most definitly cause severe portability and security issues
>> on several operating systems, in particular on modern ones.
>>
>> What do you think?
>>
>> Should i do testing without these three modules on OpenBSD, Linux,
>> and various versions of Solaris to see if that improves matters?

> I think that unless you find out that the code using %n is really
> compiled and used on one of your build, it's not worth spending time on
> that.

I will re-test ASAP with the upcoming libc change; to save time,
i will do so without waiting for that change to be committed.

Yours,
  Ingo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]