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

[Ingo Schwarze]

Hi Ralph,

i agree with (almost) all you are saying in this mail.

Ralph Corderoy wrote on Mon, Nov 08, 2021 at 02:05:00PM +0000:
> Ingo Schwarze wrote:

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

> True in general.  We all hate wading through layers of needless
> abstraction which is a waste of effort to memorise.  But an accepted
> exception is a function which provides the same interface as the
> standard one except it never returns an error.  Instead it will cause
> the program to exit.

Indeed.

> These must be named after the original to aid comprehension:
> malloc(3) is typically wrapped as xmalloc(3) or emalloc(3).

In fact, mandoc(1) does exactly that: as an internal API, it provides
and consistently uses mandoc_malloc().  The naming is slightly
awkward, maybe xmalloc() would have been a better name, but it's
probably not worth the churn of changing it.

> Instead of returning an error, the function can write(2) a diagnostic
> to stderr and abort(3) to give the user something to diagnose.

Actually, just exit(1) is better than abort().  The abort() facility
is intended for catching violations of invariants (i.e., catching
bugs).  It should not be called for normal runtime failures.

> This isn't as good as error returns rippling all the way up the
> call stack, carefully handled appropriately at each level,

That is needed in (public) libraries and occasionally useful in some
application code, but not necessary in *all* application code.

> but for gradual improvement of legacy code it's achievable.

Yes, in that role, it might even be accaptable as a temporary stopgap
in a public library.

> Analysing all call sites, correctly determine the right action,
> having regression tests to confirm accuracy, etc., means the
> improvement never happens.

Yes, there is a danger of that consequence.

Then again, changing from pure malloc(3) to xmalloc() also requires
proceeding carefully and not blindly by script because there might
be rare instances of malloc(3) calls where handling failure and
*not* exiting the program right away could be functionally
important.  It is rare though, no such case exists in mandoc,
for example.  So you are almost certainly right that a large-
scale change of buggy and inconsistent code to xmalloc() is simpler,
quicker, and less risky than directly to proper malloc(3) idioms.

> It doesn't matter that a ‘library’ call aborts in groff's case as
> it's not like ed(1) losing one's magnum opus by aborting.

And besides, libgroff is only an internal library, not a public
one, so it can be regarded in the same way as application code.

> And by moving to
> estrdup(), say, then all the sites which need examining in a future
> round of improvement can be easily found compared with sticking to
> strdup() and some of them having been vetted and some not.

True in general, but not an issue in this case: the groff codebase
does not call strdup(3) right now.  So if Branden proceeds as he
plans, it will be clear that all sites calling strdup(3) are
already audited and all still calling strsave() have not yet been
checked.

Yours,
  Ingo