FreeBSD removed the HOST_NAME_MAX define in an attempt to have donwstream
consumers use the sysconf(3) API. This change has lead to various consumers
erroneously using the new API. Either mistaking _SC_HOST_NAME_MAX, which is
a parameter to sysconf(3) as the replacement of HOST_NAME_MAX or by using
sysconf(_SC_HOST_NAME_MAX) as a compile time constant which in turn can
lead to accidental introduction of variable length arrays (VLA).
The decision has been documented in /usr/include/sys/syslimits.h
/* * We leave the following values undefined to force applications to either * assume conservative values or call sysconf() to get the current value. * * HOST_NAME_MAX * * (We should do this for most of the values currently defined here, * but many programs are not prepared to deal with this yet.) */
The following snippet of code demonstrates _POSIX_HOST_NAME_MAX which is the
equivalent of HOST_NAME_MAX on most platforms and the confusion between _SC_HOST_NAME_MAX
and sysconf(_SC_HOST_NAME_MAX):
$ cat hostname.c #include <unistd.h> #include <stdio.h> #include <limits.h> int main(int argc, char **argv) { printf("_POSIX_HOST_NAME_MAX=%d\n", _POSIX_HOST_NAME_MAX); printf("_SC_HOST_NAME_MAX=%d\n", _SC_HOST_NAME_MAX); printf("sysconf(_SC_HOST_NAME_MAX)=%ld\n", sysconf(_SC_HOST_NAME_MAX)); return 0; }
Output:
$ ./hostname _POSIX_HOST_NAME_MAX=255 _SC_HOST_NAME_MAX=72 sysconf(_SC_HOST_NAME_MAX)=255 $
By accident _SC_HOST_NAME_MAX has a value large enough to not be spotted in most 'quick tests' people
might make.
This review addresses one of the places of misuse I found. Stay tuned as more will follow.
When committing any of these changes please credit the work as:
Sponsored by: Fudo Security
One of such consumers is port devel/electron11 where a patch lifted from www/chromium introduces
an error:
files/patch-components_sync__device__info_local__device__info__util__linux.cc 7:- char hostname[HOST_NAME_MAX]; 8:- if (gethostname(hostname, HOST_NAME_MAX) == 0) // Success. 9:+ int len = sysconf(_SC_HOST_NAME_MAX); 11:+ if (gethostname(hostname, _SC_HOST_NAME_MAX) == 0) // Success.
The diff goes out of it's way to use sysconf(3) and introduces a variable length
array (VLA). It then fails to use the computed length in call to gethostname passing
in an accidental value '72'.
The proper non-intrusive fix for this codebase is to use _POSIX_HOST_NAME_MAX.
Interestingly, there was a different approach in www/chromium in another path in the same port:
files/patch-components_policy_core_common_cloud_cloud__policy__util.cc 35:+ char hostname[MAXHOSTNAMELEN]; 36:+ if (gethostname(hostname, MAXHOSTNAMELEN) == 0)
which uses MAXHOSTNAMELEN instead of HOST_NAME_MAX obtained indirectly (not present
in the patched file) by using sys/params.h in the source tree. This patch however has not
been copied to this port. Which makes me wonder if it is also really needed in Chromium and
if so then why is it not needed here?
In the spirit of least intrusion into a fragile and important codebase like Chromium I'm
opting for the smallest change of using _POSIX_HOST_NAME_MAX instead of sysconf(3) and
not changing MAXHOSTNAMELEN as that is already a tested change.
Suggested commit message:
commit a9e5fa29e675a161a170594446f01e86ad788a54 (HEAD -> main)
Author: Adam Wolk <a.wolk@fudosecurity.com>
Date: Fri Apr 30 22:05:48 2021 +0200
devel/electron11: fix obtaining HOST_NAME_MAX
Using sysconf(3) API lead to accidental introduction of variable length arrays
(VLA) in the port. Additionally gethostname(3) has been passed _SC_HOST_NAME_MAX
incorrectly as the HOST_NAME_MAX length.
Fall back to using _POSIX_HOST_NAME_MAX as the remaining code is not ready for
introducing sysconf(3) as a patch.
Bump PORTREVISION to rebuild with the new patch.
The patch was lifted from www/chromium which has a similar diff pending.
Sponsored by: Fudo Security