Re: [PATCH 0/7] various build fixes + OpenBSD compat, [PATCH 7/7] xap_helper.h: fix double-free on OpenBSD hdestroy

["Štěpán Němec" <stepnem@smrk.net>]

On Wed, 30 Aug 2023 05:10:38 +0000
Eric Wong wrote:

> 7/7 had me shaking my head for a bit, but I'd rather pay
> that price than introduce weirdness with C++ templates.

On Wed, 30 Aug 2023 05:10:45 +0000
Eric Wong wrote:

> hdestroy on OpenBSD assumes each key in the table can be freed,
> so use strdup to fulfil that requirement.
>
> This behavior differs from tested behavior on glibc and FreeBSD,
> as well as what I can see from reading the musl and NetBSD
> source code.  OpenBSD may be the only relevant OS which requires
> this workaround.

I guess this won't help any, but it caught my attention and I've already
done the digging, so I might as well share the findings:

OpenBSD actually copied this behavior with the initial import (of
hcreate.c) from NetBSD:

  $OpenBSD: hcreate.c,v 1.1 2004/06/24 04:43:33 millert Exp $
  https://github.com/openbsd/src/commit/46f8fdc8de36
  Commit: millert <millert@openbsd.org>
  CommitDate: Thu Jun 24 04:43:33 2004 +0000

  Working hcreate(3) et al from NetBSD (cgd) via ray at cyth dot
  net. Now passes the regress tests.

NetBSD only changed the default to match FreeBSD in 2014, and also added
hdestroy1 to allow caller control of element freeing:

  $NetBSD: hcreate.c,v 1.9 2014/07/20 13:34:17 christos Exp $
  https://github.com/netbsd/src/commit/3b1b85d301bf6e608ce20b4152
  Commit:     christos <christos@NetBSD.org>
  CommitDate: Sun Jul 20 13:34:17 2014 +0000

  Our hdestroy implementation was non-conformant because it freed the
  key of each entry. Add a new function hdestroy1 that allows the user
  to control what gets freed. Pointed out by Pedro Giffuni at FreeBSD.

That said, I'm not sure what "non-conformant" in the NetBSD commit
message refers to.  POSIX doesn't say anything on the matter
https://pubs.opengroup.org/onlinepubs/9699919799/functions/hcreate.html
and there don't seem to be any other relevant standards.

The NetBSD original (with the unconditional freeing) was apparently a
from-scratch implementation from 2001:

/*
 * hcreate() / hsearch() / hdestroy()
 *
 * SysV/XPG4 hash table functions.
 *
 * Implementation done based on NetBSD manual page and Solaris manual page,
 * plus my own personal experience about how they're supposed to work.
 *
 * I tried to look at Knuth (as cited by the Solaris manual page), but
 * nobody had a copy in the office, so...
 */

In any case, it does suck that OpenBSD is now the odd one out, but
having no experience with the interface myself, I have no opinion on
which behavior makes more sense.

Thanks for working on these (though I've had no complaints regarding the
actual functionality I need on OpenBSD so far) and let me know if you
want me to test something.  (With cindex.t out of the way (says 'ok'
now, plus a few W: reaped unknown PID), "make test" now gets stuck on
lei-import-http.t for me (with or without this series; still the same
test instance); I haven't seen kqnotify.t fail in the few runs I did.)

-- 
Štěpán