Hello,
Stefan Esser wrote:
> --- usr.bin/mandoc/mansearch.c
> +++ usr.bin/mandoc/mansearch.c
> @@ -220,7 +220,7 @@
> if (cur && search->firstmatch)
> break;
> }
> - if (res != NULL)
> + if (res != NULL && *res != NULL)
> qsort(*res, cur, sizeof(struct manpage), manpage_compare);
> if (chdir_status && getcwd_status && chdir(buf) == -1)
> warn("%s", buf);
I like the general idea and i have committed it to OpenBSD and bsd.lv
with one tweak, see the committed patch below.
My rationale is that sorting an array of zero or one elements
is useless, so it makes the code easier to read for human auditors
if we skip sorting in that case.
My rationale for the tweak is that for res != NULL,
(cur > 0) == (*res != NULL)
is an invariant of the mansearch() function. Should that invariant
ever be violated by future changes to the code, i consider it
beneficial for the program to segfault by accessing a NULL pointer
rather than silently returning inconsistent data like (*res==NULL,
*sz>0) to the caller. That seems the safer choice. Then again,
is is probably not important enough to assert(3). Simply using
"cur > 1" feels like a good compromise between readability and
providing some chance of a crash (without additional code) if the
invariant should ever get broken.
Mark Millard wrote on Wed, Jan 12, 2022 at 03:00:31AM -0800:
> ISO/IEC 9899:2011 (E) is not explicit about such things for
> qsort, nor is POSIX as I remember: POSIX states that in cases
> of disagreement it defers to a C standard, if I remember right.
>From looking at both standards, i came to the same conclusion:
It seems to me that neither standard defines what shall happen
if base == NULL && nmemb = 0.
> But ISO/IEC 9899:2011 (E) is somewhat explicit for qsort_s:
> (parameters: base, nmemb, size, and compar in that order)
I believe that is irrelevant for mandoc because it does not
use qsort_s(3). It is probably also irrelevant for the OpenBSD
C library because that does not provide qsort_s(3).
[...]
> As qsort does not return a value, any rejection of such a
> combination for qsort would be in a more drastic form, making
> such an unlikely choice. (qsort is not documented to assign
> errno either.)
>
> So I would expect qsort to avoid involving undefined behavior
> when nmemb==0 && (base==NULL||compar==NULL) but to not reject
> the condition. I do not take doing a well-defined "no-op" as
> a rejection for my wording here.
I believe the best course for qsort(3) is to silently do nothing
if nmemb == 0 and to not access base or compar in that case.
The alternative of terminating the application program with a segfault
might perhaps break some application programs, and i'm not sure that
would provide any significant benefit.
Yours,
Ingo
Log Message:
-----------
Only sort the result array if it contains more than one element,
making the mansearch() function easier to read for human auditors.
No functional change on OpenBSD.
As observed by Mark Millard <marklmi at yahoo dot com>, neither the
latest version of POSIX 2008 nor C11 defines what qsort(3) should do
for base == NULL && nmemb == 0.
My impression is it is indeed undefined behaviour because the
standards say that base shall point to an array, NULL does not point
to an array, and while there is special wording saying that compar()
shall not be called if nmemb == 0, i fail to see any similar wording
stating that base shall not be accessed if nmemb == 0.
Consequently, this patch is also likely to improve standard conformance
and portability.
Minor issue found by Stefan Esser <se at FreeBSD> with UBSAN.
He sent a patch to bugs@, but my patch differs in a minor way.
Modified Files:
--------------
mandoc:
mansearch.c
Revision Data
-------------
Index: mansearch.c
===================================================================
RCS file: /home/cvs/mandoc/mandoc/mansearch.c,v
retrieving revision 1.82
retrieving revision 1.83
diff -Lmansearch.c -Lmansearch.c -u -p -r1.82 -r1.83
--- mansearch.c
+++ mansearch.c
@@ -220,7 +220,7 @@ mansearch(const struct mansearch *search
if (cur && search->firstmatch)
break;
}
- if (res != NULL)
+ if (res != NULL && cur > 1)
qsort(*res, cur, sizeof(struct manpage), manpage_compare);
if (chdir_status && getcwd_status && chdir(buf) == -1)
warn("%s", buf);
