Re: Use `strsave()`, not `strdup()`.

[G. Branden Robinson]

Hi Ingo,

At 2021-11-07T14:57:28+0100, Ingo Schwarze wrote:
> >     [libgroff,pic]: Use `strsave()`, not `strdup()`.
> 
> That's a bad idea.
> The opposite should be done, see below for multiple reasons.
[...]
> So even though strdup(3) was invented for BSD in 1982, it wasn't
> available in the C library (even in BSD) before 1990.  Other systems
> may have adopted it even later.  I dimly remember having heard that
> some implementations were buggy in the early 1990ies (hard to believe
> given how simple the function is - but see below...), and a quick web
> search actually turned up a hilarious, pre-historic bug report on very
> ancient 32bit Microsoft Windows:
[...]
> When James Clark started the groff project in 1989, he definitely
> couldn't rely on strdup(3) yet.  But it is needless to say that all
> the above concerns have become irrelecant more than a decade ago, or
> maybe even two decades ago.  POSIX (SUSv2/XPG5) requires strdup(3)
> since at least 1997.

The Linux man-pages version of the strdup(3) page did not have a history
as extensive as yours.  The function has been available in every C
environment I've used since I first learned the language, which isn't
_quite_ as far back as 1989.  That's why I reached for it unthinkingly
when I made the changes that I reverted above.

> >     but the code has been consistent.  This change fixes `strdup()`s
> >     I introduced in ee0942bd (2 November) and 423e3c0b (28 September),
> >     respectively.
> 
> That's definitely making matters worse rather than better:
> 
>  1. strsave() contains a severe bug.  If malloc(3) fails, the
>     function segfaults.  So every time you call strsave(), that's
>     a place where groff(1) may crash in a non-deterministic manner.

Hrm, yes.

        char *strsave(const char *s)
        {
          if (s == 0)
            return 0;
          char *p = (char*)malloc(strlen(s) + 1);
          strcpy(p, s);
          return p;
        }

Barrelling blindly into strcpy() like that is truly bad.

>  2. strsave() contains a highly dangerous idiom.  When called with
>     a NULL pointer, it does *NOT* crash (as it should!) but instead
>     returns to the caller.

I'm not quite with you here.  That's what strdup() and malloc()
themselves do.  The idea behind strsave() was to replace strdup() under
a different name.  Why should it abort()?

Moreover, if it does abort(), we're depriving our caller of the
opportunity to do so.  My preference is generally to issue diagnostics
from executables, not libraries (groff's design and its use of
pre-exception C++ makes this principle hard to adhere to sometimes).

The pattern I've adopted in other places is this:

1. Call the dynamically allocating function.
2. Check the pointer it returns for nullity.
3. If that pointer is null, write a diagnostic with fputs(..., stderr))
   (which doesn't allocate memory as printf() can and often will if you
   use a format string with a conversion specifiers ("%") in it), then
   exit() or abort() as appropriate to the project.

>     That is very dangerous because it can
>     hide bugs in the calling code.  Even though the caller is
>     obviously in an invalid state handling invalid pointers, it
>     will prod on doing god knows what.  Executing more code in an
>     uncontrolled manner after it is already known that something
>     went wrong is never a good idea.

The foregoing is my idea of a controlled crash.  I hope you will point
out how it can be improved.

>  3. Using your own replacement functions for Standard C and/or
>     POSIX functions is a bad idea because:
> 
>  3.1. It makes the code harder to read and audit for experienced
>       programmers.  Instead of being able to use their knowledge
>       of standards, they have to figure out what your private
>       functions do and how they are meant to be used, slowing
>       down the process of understanding the code and making it
>       more likely that auditors miss bugs when inspecting the
>       code and that maintainers introduce new bugs when trying
>       to change and improve the code.

Yes, I agree.  It was a long time before I found out that strsave() was
(meant to be) a strdup() replacement.

It is for this same reason that, in August, I killed off groff's
`a_delete` and `ad_delete` functions, which were stand-ins for a C++
runtime language feature, deeper even than a standard library function.
It evidently only existed to support "pre-ARM" C++ compilers.

> So, i think the groff codebase should be audited for all uses of
> strsave() - because every one of them is a bug site.  The auditor
> should understand what every call does, replace it with strdup(3), and
> test the code afterwards.  This needs to be done with the brain
> switched on and *NOT* with Coccinelle and not in in some other
> mechanical way because it is likely that many call sites are buggy,
> too, and that different changes are needed at different sites - for
> example, some sites will likely need a NULL check while others will
> not.

Okay.  There are at present 127 such call sites.

This will take time.

> Second, even if it could, the error message is extremely misleading.
> The actual problem is memory exhaustion, and you go and tell the user
> "unable to determine a paper format"?  You are sending that poor guy
> on a wild goose chase!

I agree, and I wrote the new diagnostic with some discomfort.  I felt
the urge for an OOM-reporting diagnostic but didn't want to start a yak
shave.

> >      printf(".lf %d %s\n", n, s);
> > -    last_filename = strdup(s);
> > +    last_filename = strsave(s);
> 
> Error checking is obviously missing here.  As long as strdup(3)
> was used, the error was totally obvious, and no auditor could
> possibly have missed it even when half asleep: unchecked malloc(3).
> With strsave(3), the error is much less obvious, and the consequences
> of the bug changed: before, the program prodded on with invalid data,
> having stored a NULL pointer in last_filename.  Now, it segfaults
> right away.  But the bug survived, it just changed colour.

Thank you for the code review.

> I think i said so before: the groff codebase is teeming with bugs.
> Look at anything, and it is almost certain that you will instantly
> find some obvious bugs.  Unfortunately, i don't have the time to do
> the badly needed audit myself.

I'm surely not the experienced auditor that you are (this is at least
the second time you've caught me failing to be militantly paranoid with
pointer handling), but until such a resource is available, I'll try to
make things better as I encounter issues.

Right away I see that `do_error_with_file_and_line`[2], if called
carefully enough through the layers of diagnostic functions above it,
and with a few slight changes, can be invoked without itself, or the
functions it calls (like `errprint()`) making recourse to dynamic
memory allocation.

I'll work on this.

I see a few tasks arising from your mail.

1. Add an Autoconf check for strdup().
2. Revert the commit, leaving only 125 strsave() call sites.
3. Revise do_error_with_file_and_line() to not use *printf().
4. Test return value from strdup(); call fatal() if it's a null pointer.

Regards,
Branden

[1] See 
https://git.savannah.gnu.org/cgit/groff.git/commit/?id=61d2307000c457db0604a7833e29ca4ad15884f7
    and the 3 parent commits.
[2] https://git.savannah.gnu.org/cgit/groff.git/tree/src/libs/libgroff/error.cpp

signature.asc
Description: PGP signature