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
