Re: Releasing groff 1.22.5?

[Bertrand Garrigues]

Hi Ingo,

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

>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
   [...]
   #else           /* AIX <= 5.1, HP-UX, IRIX, OSF/1, Solaris <= 9, BeOS */

                   fbp[1] = '%';
                   fbp[2] = 'n';
                   fbp[3] = '\0';
   #endif

Does your build bump into the code after the #else?

> 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).

> 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.

[...]
> 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.  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.

> 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.

Regards,
Bertrand