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

[Ingo Schwarze]

Hi Brandon,

thanks for your nice reply; i'm snipping what i agree with.

G. Branden Robinson wrote on Mon, Nov 08, 2021 at 06:55:55AM +1100:

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

Not so for me; i learned C with a Microsoft Visual Studio compiler
in the late 1980ies.   Even though i was coming from the
  https://en.wikipedia.org/wiki/HP_9800_series#HPL
programming language, which is not exactly the most beautiful language
either, the experience with the Microsoft Visual Studio C compiler
was frustrating to the point that after that, i avoided the C language
for about a decade and used FORTRAN 77 almost exclusively until the end
of the 1990ies, which served me very well, with a few short episodes of
trying Borland Pascal in parallel, which was also highly frustrating:
i didn't know back then that you can't really take Pascal seriously as
a programming language, and even less so in those days.

I very much deplore now that i never used the opportunity to try a
real C compiler when i worked extensively on HP-200 workstations
in the mid-1980ies...  :-/

> That's why I reached for it unthinkingly
> when I made the changes that I reverted above.

That was a *very* reasonable thing to do; be proud of it rather
than ashamed!

[...]
> At 2021-11-07T14:57:28+0100, Ingo Schwarze wrote:

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

I think you misunderstood what i tried to say.  If you call strdup(NULL),
the result is unspecified by POSIX.  Implementations usually segfault,
and that is what i think they should indeed do.  In any case, calling
strdup(NULL) is a severe bug in any program.

The behaviour of malloc(NULL) is also not specified by POSIX.
Casting NULL to size_t will usually yield 0 (not sure it is
required to, but i would be surpised of any real world implementation
did anything else), and malloc(0) is explicitly implementation-
defined according to POSIX.

> The idea behind strsave() was to replace strdup() under
> a different name.  Why should it abort()?

If the intention of strsave() was to replace strdup(), then
the correct implementation would have been

        size_t sz = strlen(s) + 1;
        char *p = malloc(sz);
        if (p != NULL)
                memcpy(p, s, sz);
        return p:

which usually segfaults is s == NULL.  Of course, the implementation
should not explicitly call assert(3).  Using assert(3) sparingly is
usually a good idea, and in a library even more so than in application
code.  If a program contains a bug that passes a NULL pointer to
a string handling (or similar) function, then it is a good thing
if that program crashes instantly with a NULL pinter dereference,
because that will get the bug fixed rather than hiding it.

> Moreover, if it does abort(), we're depriving our caller of the
> opportunity to do so.

That does not make any sense to me.  Calling strdup(NULL) is a programming
bug.  You must not attempt to write code to handle programming bugs.
The purpose of error handling in code is to handle run-time failures
(like unsuitable input data or memory exhaustion), not to handle
programming bugs.

> My preference is generally to issue diagnostics
> from executables, not libraries

Yes, of course.

> (groff's design and its use of pre-exception C++ makes this
> principle hard to adhere to sometimes).

Not really.  Proper error handling in well-designed application code
absolutely does not need C++ exceptions.

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

It is true that POSIX allows printf(3) to fail with ENOMEM, but unless
you use %ls, that's a theoretical issue.  I don't think real-world
implementations allocate memory if you refrain from relying on wide
character to multibyte character conversions.  Why should they?

>    then exit() or abort() as appropriate to the project.

Yes.  A good idiom is

        if ((p = malloc(sz)) == NULL)
                err(1, NULL);

if you have the non-standard <err.h> available or equivalently,

        if ((p = malloc(sz)) == NULL) {
                perror(__progname);
                exit(1);
        }

if you don't.  Note that saying where malloc(3) failed - in which
function, for which call - is pointless.  It does not help the
user and usually isn't deterministic either.  The message

  __progname: Cannot allocate memory

contains all information the user needs.

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

Yes, i noticed, and i liked that, even though i did not speak up
nor review your code changes because i rarely use C++ and could
not hope to add much insight to that work.

[...]
> Okay.  There are at present 127 such call sites.
> 
> This will take time.

Absolutely.  Code auditing must not be done in a hurry, and refactoring
even less so.  If you really want to work on that, it is highly
appreciated.

[...]
> I'll try to make things better as I encounter issues.

Please allow me to call that "best practice".  :-)

> Right away I see that `do_error_with_file_and_line`[2],

I think printing the names of source code files, the names of functions
in the source code, or line numbers of call sites in source code files is
bad practice.  Such information is totally irrelevant for normal users,
it is confusing and distracting, and it just makes the program look as
if it were written for nerds rather than for users.  It does not even
help developers.  If you are hunting bugs, sometimes the functions to
look at are obvious anyway if you know the code base in question, and
in other cases, you usually need a debugger anyway.

> if called carefully enough through the layers of diagnostic functions
> above it,

Indeed.  Error reporting is one of the subject areas where almost
everybody falls into the trap of over-engineering.  It happened to
myself, too, in multiple of my projects in the past.

> and with a few slight changes, can be invoked without itself,

I don't think i understand what you mean by "without itself".

> or the functions it calls (like `errprint()`) making recourse to
> dynamic memory allocation.

I don't see yet what in errprint() might allocate memory.
Of course, src/libs/libgroff/itoa.c is a shitshow (even if it
is correct, which i did not check).  Just using printf("%i%u")
would be so much clearer, cleaner, and reduce the risk of bugs.

> I'll work on this.

Thanks.

> I see a few tasks arising from your mail.
> 
> 1. Add an Autoconf check for strdup().

No, please don't.  Just use strdup(3), period.  Seriously, POSIX
has been requiring it for almost 25 years now.

> 2. Revert the commit, leaving only 125 strsave() call sites.

Heh.  :-)

But do fix the bugs in the code touched by that commit.  ;-)

> 3. Revise do_error_with_file_and_line() to not use *printf().

Why?  It does not use %ls.  So what exactly is the problem?

You don't really fear that

  fprintf(stderr, "%s:", FOO);

might somehow end up in malloc(3), or do you?

Of course, using fputs(3) would be fine, too.

> 4. Test return value from strdup(); call fatal() if it's a null pointer.

Right.
The right idiom probably is:
  fatal("%1", strerror(errno)); 
unless you wan to clean up all that overengineered mess in and around
the file src/libs/libgroff/error.cpp and use standard facilities instead.

Note that
  fatal("strdup: %1", strerror(errno));
would be pointless.  It is completely irrelevant for the user whether
it was malloc(3) or strdup(3) or whatever other allocating function
that failed.  All that matters is "Cannot allocate memory".

Yours,
  Ingo