From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 7EB081F45F for ; Tue, 7 May 2019 00:28:00 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH] spawn (Inline::C): fix off-by-one error Date: Tue, 7 May 2019 00:28:00 +0000 Message-Id: <20190507002800.10069-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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(-) diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm index 7b0f3bd..66b916d 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"); -- EW