From 3bd8bec866f5540b6dce33ffcaaf4714249dcb22 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Mon, 6 May 2019 23:42:31 +0000 Subject: spawn (Inline::C): fix off-by-one error Noticed while testing on FreeBSD 11.2 amd64 with the optional Inline::C extension using clang 6.0.0. The end result on FreeBSD was spawning processes failed badly and things were immediately unusable with this enabled. av_len is a misleading API, and I failed to read the API comments in perl:/av.c which state: > Note that, unlike what the name implies, it returns > the highest index in the array, so to get the size of > the array you need to use "av_len(av) + 1". > This is unlike "sv_len", which returns what you would expect. If this bug affected anybody, it would've only affected users using both the optional Inline::C module AND set the PERL_INLINE_DIRECTORY environment variable. That said, I've never seen any evidence of it on Debian GNU/Linux + gcc on any x86 variant. That includes full 64-bit systems, a full 32-bit system, a 64-bit system with 32-bit userspace, across multiple gcc versions since 2016. --- lib/PublicInbox/Spawn.pm | 47 ++++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-) (limited to 'lib') diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm index 7b0f3bdd..66b916df 100644 --- a/lib/PublicInbox/Spawn.pm +++ b/lib/PublicInbox/Spawn.pm @@ -26,22 +26,35 @@ my $vfork_spawn = <<'VFORK_SPAWN'; #include #include #include -#include -#include -#include +#include -#define AV_ALLOCA(av, max) alloca((max = (av_len((av)) + 1)) * sizeof(char *)) +/* some platforms need alloca.h, but some don't */ +#if defined(__GNUC__) && !defined(alloca) +# define alloca(sz) __builtin_alloca(sz) +#endif -static void av2c_copy(char **dst, AV *src, I32 max) -{ - I32 i; +#include +#include - for (i = 0; i < max; i++) { - SV **sv = av_fetch(src, i, 0); - dst[i] = sv ? SvPV_nolen(*sv) : 0; - } - dst[max] = 0; -} +/* + * From the av_len apidoc: + * Note that, unlike what the name implies, it returns + * the highest index in the array, so to get the size of + * the array you need to use "av_len(av) + 1". + * This is unlike "sv_len", which returns what you would expect. + */ +#define AV2C_COPY(dst, src) do { \ + I32 i; \ + I32 top_index = av_len(src); \ + I32 real_len = top_index + 1; \ + I32 capa = real_len + 1; \ + dst = alloca(capa * sizeof(char *)); \ + for (i = 0; i < real_len; i++) { \ + SV **sv = av_fetch(src, i, 0); \ + dst[i] = SvPV_nolen(*sv); \ + } \ + dst[real_len] = 0; \ +} while (0) static void *deconst(const char *s) { @@ -86,15 +99,11 @@ int pi_fork_exec(int in, int out, int err, const char *filename = SvPV_nolen(file); pid_t pid; char **argv, **envp; - I32 max; sigset_t set, old; int ret, errnum; - argv = AV_ALLOCA(cmd, max); - av2c_copy(argv, cmd, max); - - envp = AV_ALLOCA(env, max); - av2c_copy(envp, env, max); + AV2C_COPY(argv, cmd); + AV2C_COPY(envp, env); ret = sigfillset(&set); assert(ret == 0 && "BUG calling sigfillset"); -- cgit v1.2.3-24-ge0c7