Message ID | PAXP251MB03177947C164F6FEF2BB97F9F9ED2@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers |
Return-Path: <newlib-bounces~patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B3BB13858D37 for <patchwork@sourceware.org>; Sun, 26 Jan 2025 11:14:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B3BB13858D37 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=HOTMAIL.IT header.i=@HOTMAIL.IT header.a=rsa-sha256 header.s=selector1 header.b=DguBCBes X-Original-To: newlib@sourceware.org Delivered-To: newlib@sourceware.org Received: from EUR02-DB5-obe.outbound.protection.outlook.com (mail-db5eur02olkn20811.outbound.protection.outlook.com [IPv6:2a01:111:f403:2e08::811]) by sourceware.org (Postfix) with ESMTPS id 97F9D3858D37 for <newlib@sourceware.org>; Sun, 26 Jan 2025 11:14:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 97F9D3858D37 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=hotmail.it Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=hotmail.it ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 97F9D3858D37 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=2a01:111:f403:2e08::811 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1737890063; cv=pass; b=rIxyYCkE2N8jFPP1rss67pJLCXAMB6wooKkE+Z+NMUtkgtMtt32PfgobU6RluBhy6Dhm2dsW9AmoCZp8RJ9TTVqeJ74w1p4m/wQYtfbn2x9bHiEdPK1gg87+AkfFcBs8K2cYEjKRw13MO11pnQN3nBWUvyQZAG0so5os3TCDbOk= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1737890063; c=relaxed/simple; bh=tbl8EBKHoxLiHhU7ykUS32XExXhRss9PczCj+symVt8=; h=DKIM-Signature:Message-ID:Date:From:Subject:To:MIME-Version; b=b3ppM/xo3hLZSLOk9P8qsYj+/0to5fQkpmj7fX7NQ7D6KoqdONZuwdBkyBSiYbreKpr6U/Z/31SGrlCTtze4dxhkD2z+uidlQOdgOIO0g0pYjrpJSJU+jGB6+VwD9o/8EBE7Hv0GfjpWQ/WCpxibXGIZLpYLlO65GzmisKK8cEo= ARC-Authentication-Results: i=2; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 97F9D3858D37 ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=cJIxw/+vXI0ViP+QGrVVwMOwIrm0NwGLsSqGGmRhwKw1iOpEfxhzAERn7XzTxRcPTaG0QnYWq/uxJbzM8PadME4uVRjxUzpQjV64EB6aj6z9MIioL+cBzxLqtZf5ywUuiv8EPEcgr2I//mRCOPHeAv0Jm2/izoWRpuVHBvy4FrFR8Ks2UcbEjtzEUvTe4cAbTmb7ZHLGYeRlx6wLj7w/uf19+XZhPWOTjvsRegBg3Sg8gudEJfhNqcqSW33szinFMADISZ7wMlllTdDr/2p9MV+D29VOpwXsTRHUS0ICtgixPbiRV0wA+7Gc6/8LOJSfM/D7kihHF/ra+lUIfqNt8g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=S8qpS9WfYQ8ReGe64zmtUEZhtZYCA5LLOkKWo69ItuA=; b=Obbqzny+/maw38box35spBvkXKbYz2a0l/0AY38BZmUB/6IJW/25CT9gKfmgNrl2mV6xohyT4rhEJvvWyppP+MgO01f/q8QQiA7o4XAKsTzcHL5woaGQBivEA77l7h3dDwhqoT6KQZd5KpI2TmhlQNv1J5VxWFSDspUNLwg0jGN6yqDWRwZHqZh/uixFyyBNFkxo7Yzrfk2M104adLMeACCEoaxxgpIYg3pldnLjZEMRTgMLce0luaKz0/Mlnhd/yPP/ZNSG25KE2uFkIELWMmmN7+Oo+MuYPAjZRJ1pk9eV0OdEg+LJL9gIPSAh5oBIMnebv5QbtyFclMj2mjpIzw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=HOTMAIL.IT; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=S8qpS9WfYQ8ReGe64zmtUEZhtZYCA5LLOkKWo69ItuA=; b=DguBCBesdo0Zz82lOzyTRFphox3m6PTmgn0FVXMV7CBqNMLJ9Dfl11kSkJIuiOFllBqio/A5BHxPI1MJN8D+F/MuJ30vS2Jxt9KeOz8fjVmHpxoGVZKebrzOgGw5e+FCGcyMIDIx9/wc29d0tGfRs8KCiO50rp9PuiJhoRofEM6AZlaAHjZv4Qo5Zog2PXxpD+avBmOwUtBjBx+o9NzqgTSWoYo/X3vEA5skm3fmJw71/8uHUBLPJyqfOaY4QBU2zYQZsqdFU5w2bU0sBpi6QsWgPLkvtWfGL/CkYgPz7DSbEmKKEL8DHMJly4HnAEaoaFC37XUVFZXQfoL3T25ROA== Received: from PAXP251MB0317.EURP251.PROD.OUTLOOK.COM (2603:10a6:102:209::22) by AS4P251MB0635.EURP251.PROD.OUTLOOK.COM (2603:10a6:20b:4be::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8377.21; Sun, 26 Jan 2025 11:14:20 +0000 Received: from PAXP251MB0317.EURP251.PROD.OUTLOOK.COM ([fe80::4be5:8de0:6cd6:4eb2]) by PAXP251MB0317.EURP251.PROD.OUTLOOK.COM ([fe80::4be5:8de0:6cd6:4eb2%6]) with mapi id 15.20.8377.021; Sun, 26 Jan 2025 11:14:20 +0000 Content-Type: multipart/mixed; boundary="------------UK88EoB1Pu009p27qqsS9Jap" Message-ID: <PAXP251MB03177947C164F6FEF2BB97F9F9ED2@PAXP251MB0317.EURP251.PROD.OUTLOOK.COM> Date: Sun, 26 Jan 2025 12:14:11 +0100 User-Agent: Mozilla Thunderbird From: Federico Terraneo <fede.tft@hotmail.it> Subject: [PATCH] Improve execl* functions Content-Language: en-US To: newlib@sourceware.org X-ClientProxiedBy: MI1P293CA0013.ITAP293.PROD.OUTLOOK.COM (2603:10a6:290:2::15) To PAXP251MB0317.EURP251.PROD.OUTLOOK.COM (2603:10a6:102:209::22) X-Microsoft-Original-Message-ID: <abdcaa2c-d975-4e0b-91af-06f582af92f9@hotmail.it> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: PAXP251MB0317:EE_|AS4P251MB0635:EE_ X-MS-Office365-Filtering-Correlation-Id: 50641f9f-8d4e-44f9-0243-08dd3dfa9073 X-Microsoft-Antispam: BCL:0; ARA:14566002|5072599009|15080799006|8060799006|19110799003|461199028|12121999004|6092099012|7092599003|440099028|13095399003|3412199025; X-Microsoft-Antispam-Message-Info: =?utf-8?q?jtpRimpu2yQhMcAIemq7Jnc+NvkCIVw?= =?utf-8?q?rl3IbB0kqYBbqDnbfEFzK7W+dK4qBiNEV8EFi655wJnicJr0/zr8rj8/BmNTPiAKP?= =?utf-8?q?I+YqONc6YBVZ0WMvgb0IXRJYfVz/cYaN+3DvJg5gqMwucAGVmNeFf6UFVivP0QOpJ?= =?utf-8?q?zQlRWJWcsANbgtzuS3x4cMYrAMTlLXjsk+EFYjBNcgiWEMIjV6bKgW0HKDR9ex2Ra?= =?utf-8?q?X3muiJJCS6l8B1L28KXvC/cCkkwE9v7788vhudQ/WbnH1kMGT6IwmqoqpZ+wt8ZcP?= =?utf-8?q?YGSQPfCnPkfgaY38stMVqihAEAkv5f2M9cCpMEyk/yQT4zkBPhFhd3lTnqbyanReA?= =?utf-8?q?6SK/ogi9mKZFafy6YRM7iZvFQNRVUQeL0fP9yvl5ZCpBu12anH8d8MUY8uzIWAvUc?= =?utf-8?q?hzo+9lsElqvHobo4ZAd7d35nP3zHIwnxyCdWyE+bWKVIll4NjvDbtQXJ7yLxDA5aT?= =?utf-8?q?v/c4r5FvAVrzfUhAfsjVlUW6Z0mR9dzSNym7NPkjh02IIReb/9NcR1ZOVuvpx6PEP?= =?utf-8?q?YKAWdZoue4yy6yGBIUZ0GCiE2CbHpO07E7kqpcMVavjNQzTjsoE1CxaxNzzDUJXYy?= =?utf-8?q?odeq9PL/8lgtoj4C9bjjJIWr00w2fMyvWlGWEvhHV+bHxtO8e+BaGkq0n3wKJWuWa?= =?utf-8?q?MxnUXAoy8AtiBxynX7PnXVpp0dvWVcMYuALnrOWUtLsocd6akQ49sF2HiT0fIYqG1?= =?utf-8?q?9UMyePj5mcf5qiIMUUPAPUmv4UDr32u3uoxu0m6YISJrfCLq/HDplG1vVqV6pTrWu?= =?utf-8?q?+FNP84+ZrTH16vn59n/zhh5KOCNfAmUfwpP+oyZcKwy6DjriCf2HmgGjIkPvdDlhu?= =?utf-8?q?LzzeO13kBeg7A5Pwjn5JLiqMSgUtUwZYFoDKtE5+m5yONhkB64p2wwLctcbkGjMwa?= =?utf-8?q?FlNgMhtcEGBAIYAj5jfTC9gqmc1k/R64xwvTqnLO/mcxxeQnpSwgcvoAO6SZ/uHcm?= =?utf-8?q?9kf24Nrme6t/sjLMdaHxeyqinu1BAyvyciozeHsPecQq3e2vRZEDZSHn5DUmxuZug?= =?utf-8?q?5M6ZAWxzP7PlJXOYS5sOF86Lf2BcSPg7E3jEGyJPCysXHsfKLMOGcCXZ0QG2wc5B6?= =?utf-8?q?CFDnIbi+F90MSvVoilVw0AMzcexIIn/Ypu5/JIggre+evlibAL9LepT5LTE=3D?= X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?q?62NcDVp6bx0dhL5Tr7DOQgiUE2OA?= =?utf-8?q?tMtXOFesFTg9L/3ERQJuvEaiILK/fHen7PQGhvxlPIVLGMaEaYhqT/n7t4b+TKpWJ?= =?utf-8?q?AXnW5I4c/hgwWoYaxrAP0jDHUV/r0iJ2QSnNhojLkyyY5kSRB6XntOeInmJ/wGDpa?= =?utf-8?q?GQodu7obqPrdkJ8Uow8yx6QFE/VUP7NUb8JXg7GXbNXicHP245YltUYgZDd35uSYk?= =?utf-8?q?T8zmGBqGyG3C4LSw6mbaopwzZ3VXEhIUhsjgY3pNfis11AlEjQSdbyHnLDMk9Y2Vg?= =?utf-8?q?CGlINH78sBWaPDB+1cBIkWL9KtAwhGvduAMeyJNkSyMCkDfrxxTjIVBSzugxTBYNy?= =?utf-8?q?Q/h97l4LPYLW82GABr209K0pVSA2gq4oweE6k82A2KYic8BO/LjG60WZIKLNtVdmd?= =?utf-8?q?NVfodwsvqfYa70wBJHOBWDxqV7RxfK8zmAxkcY2tSaLTckvNqm7Skb4c3HvDfNzUA?= =?utf-8?q?Re4kAnXjp+FVLz8QvMJo22dup5OgMNVaB3r32rwgRTA3qk9YzOVYKq2WjEG8FB7zX?= =?utf-8?q?Tgl8lbdwMPL3J/Kh+8/KG9Qc/1j428VVk0vWR1izjtIrAGgV0gHH/ox0ILQKETEom?= =?utf-8?q?fcHO1EOj1bE6bZSw6Xl7UMpc5/AwS2S6KIV3pQwY023/ruVKmK7mH4c0ONpQdnKcN?= =?utf-8?q?91fXBg0JgvTzZNLPy5fhMNeqot5ROwq/bR/aB9TcjlYJvuQseDIJP67jwiHUtZN6o?= =?utf-8?q?gT91H3gI1bf6cKkW6JXTJgT10Ye1LxAlTGWWyDR7X2/uxYtmMFglDPQdC6r/SewIE?= =?utf-8?q?C0P5OMOJU+N2r/yb4SnBiH5aTG1paEZr+lr3ZATk4FuaOfmXmG4A2HnCofNHB6zrw?= =?utf-8?q?yUjCz7RVcHjPJ5NT5GqHSpiurv3MCgYOkS+2woOjFOQ1yvtt5KE+2Anb+UOxfybiq?= =?utf-8?q?BsnE1sDsdvwTj6sfp0cg1K8dRnjnBHrtTj0DH/j7xcXSCNVZg2xVpYIZOHJgxz8D5?= =?utf-8?q?sEZY8fKwQn1bRG+rXz10uvB4GDNq912T2dZMntNJEZe+GYNemjqEq/gpSvCcgbZKD?= =?utf-8?q?pXswkKCwt6G/VD7rD/S1s8HCkcvgA4ZrZXbwg1Ty/Otx4DHQZ2SpPTdJaDiepPEI+?= =?utf-8?q?la5zLlx4USsLEKHwpxK5dR4L7+D5AW7LwxiWaNm5RxRcBIkOZK+hH2Zlw86+rdzYj?= =?utf-8?q?MMv4oJDqBKmtGehr3n65DpOp/KsMmUXaCB1tSBB2IG7U0fRUoge6QT48wdZMc=3D?= X-OriginatorOrg: sct-15-20-4755-11-msonline-outlook-00b75.templateTenant X-MS-Exchange-CrossTenant-Network-Message-Id: 50641f9f-8d4e-44f9-0243-08dd3dfa9073 X-MS-Exchange-CrossTenant-AuthSource: PAXP251MB0317.EURP251.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2025 11:14:20.2774 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS4P251MB0635 X-Spam-Status: No, score=-13.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: newlib@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Newlib mailing list <newlib.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/newlib>, <mailto:newlib-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/newlib/> List-Post: <mailto:newlib@sourceware.org> List-Help: <mailto:newlib-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/newlib>, <mailto:newlib-request@sourceware.org?subject=subscribe> Errors-To: newlib-bounces~patchwork=sourceware.org@sourceware.org |
Series |
Improve execl* functions
|
|
Commit Message
Federico Terraneo
Jan. 26, 2025, 11:14 a.m. UTC
Hi, I propose the following patch to the execl* functions in newlib that does the following: - it returns an error if more than the maximum number of arguments are passed instead of corrupting the stack - it allows to tweak the maximum number of arguments at the time of compiling newlib by defining the ARG_NUM_MAX macro This patch is originally meant for the Miosix OS to allow reducing the maximum number of arguments with the intent of reducing the stack usage of the execl* functions in low memory microcontrollers, but I think it is of general applicability and can of course be useful also to increase this value for desktop-class targets. Best regards, Federico Terraneo
Comments
Hi Federico, I'm in no possition to either accept or reject this patch, but here is what I've seen when revieing you patch. On 2025-01-26 12:14, Federico Terraneo wrote: > Hi, > I propose the following patch to the execl* functions in newlib that > does the following: > - it returns an error if more than the maximum number of arguments are > passed instead of corrupting the stack > - it allows to tweak the maximum number of arguments at the time of > compiling newlib by defining the ARG_NUM_MAX macro > > This patch is originally meant for the Miosix OS to allow reducing the > maximum number of arguments with the intent of reducing the stack usage > of the execl* functions in low memory microcontrollers, but I think it > is of general applicability and can of course be useful also to increase > this value for desktop-class targets. > > Best regards, > Federico Terraneo > > > > 0001-Fix-stack-overflow-in-exec-add-ARG_NUM_MAX.patch > > From 9d06b54b1f2a2957bc40af10c6dc09867662325f Mon Sep 17 00:00:00 2001 > From: Terraneo Federico <fede.tft@miosix.org> > Date: Sat, 25 Jan 2025 11:51:15 +0100 > Subject: [PATCH] Fix stack overflow in exec*; add ARG_NUM_MAX > > --- > newlib/libc/posix/execl.c | 19 +++++++++++++++---- > newlib/libc/posix/execle.c | 17 ++++++++++++++--- > newlib/libc/posix/execlp.c | 19 +++++++++++++++---- > 3 files changed, 44 insertions(+), 11 deletions(-) > > diff --git a/newlib/libc/posix/execl.c b/newlib/libc/posix/execl.c > index c3b4e55bd..044205cc6 100644 > --- a/newlib/libc/posix/execl.c > +++ b/newlib/libc/posix/execl.c > @@ -7,6 +7,7 @@ > > #include <_ansi.h> > #include <unistd.h> > +#include <errno.h> > > /* Only deal with a pointer to environ, to work around subtle bugs with shared > libraries and/or small data systems where the user declares his own > @@ -16,6 +17,10 @@ static char ***p_environ = &environ; > > #include <stdarg.h> > > +#ifndef ARG_NUM_MAX > +#define ARG_NUM_MAX 256 > +#endif > + > int > execl (const char *path, > const char *arg0, ...) > @@ -24,14 +29,20 @@ execl (const char *path, > { > int i; > va_list args; > - const char *argv[256]; > + const char *argv[ARG_NUM_MAX]; > > va_start (args, arg0); > argv[0] = arg0; > i = 1; > - do > - argv[i] = va_arg (args, const char *); > - while (argv[i++] != NULL); > + do { > + if(i>=ARG_NUM_MAX) if (i >= ARG_NUM_MAX) (Add the missing spaces). > + { > + va_end (args); > + errno=E2BIG; errno = E2BIG; (Add the missing spaces). Same applies to the other files in the patch. Kind regards, Torbjörn > + return -1; > + } > + argv[i] = va_arg (args, const char *); > + } while (argv[i++] != NULL); > va_end (args); > > return _execve (path, (char * const *) argv, *p_environ); > diff --git a/newlib/libc/posix/execle.c b/newlib/libc/posix/execle.c > index 34f0ea373..7a25ae8ef 100644 > --- a/newlib/libc/posix/execle.c > +++ b/newlib/libc/posix/execle.c > @@ -7,10 +7,15 @@ > > #include <_ansi.h> > #include <unistd.h> > +#include <errno.h> > > > #include <stdarg.h> > > +#ifndef ARG_NUM_MAX > +#define ARG_NUM_MAX 256 > +#endif > + > int > execle (const char *path, > const char *arg0, ...) > @@ -20,14 +25,20 @@ execle (const char *path, > int i; > va_list args; > const char * const *envp; > - const char *argv[256]; > + const char *argv[ARG_NUM_MAX]; > > va_start (args, arg0); > argv[0] = arg0; > i = 1; > - do > + do { > + if(i>=ARG_NUM_MAX) > + { > + va_end (args); > + errno=E2BIG; > + return -1; > + } > argv[i] = va_arg (args, const char *); > - while (argv[i++] != NULL); > + } while (argv[i++] != NULL); > envp = va_arg (args, const char * const *); > va_end (args); > > diff --git a/newlib/libc/posix/execlp.c b/newlib/libc/posix/execlp.c > index b845c88c5..a437a56f0 100644 > --- a/newlib/libc/posix/execlp.c > +++ b/newlib/libc/posix/execlp.c > @@ -7,10 +7,15 @@ > > #include <_ansi.h> > #include <unistd.h> > +#include <errno.h> > > > #include <stdarg.h> > > +#ifndef ARG_NUM_MAX > +#define ARG_NUM_MAX 256 > +#endif > + > int > execlp (const char *path, > const char *arg0, ...) > @@ -19,14 +24,20 @@ execlp (const char *path, > { > int i; > va_list args; > - const char *argv[256]; > + const char *argv[ARG_NUM_MAX]; > > va_start (args, arg0); > argv[0] = arg0; > i = 1; > - do > - argv[i] = va_arg (args, const char *); > - while (argv[i++] != NULL); > + do { > + if(i>=ARG_NUM_MAX) > + { > + va_end (args); > + errno=E2BIG; > + return -1; > + } > + argv[i] = va_arg (args, const char *); > + } while (argv[i++] != NULL); > va_end (args); > > return execvp (path, (char * const *) argv);
Hi Federico, On Jan 26 12:14, Federico Terraneo wrote: > Hi, > I propose the following patch to the execl* functions in newlib that does > the following: > - it returns an error if more than the maximum number of arguments are > passed instead of corrupting the stack > - it allows to tweak the maximum number of arguments at the time of > compiling newlib by defining the ARG_NUM_MAX macro > > This patch is originally meant for the Miosix OS to allow reducing the > maximum number of arguments with the intent of reducing the stack usage of > the execl* functions in low memory microcontrollers, but I think it is of > general applicability and can of course be useful also to increase this > value for desktop-class targets. > > Best regards, > Federico Terraneo > > I'm sorry, but I don't think this is the right thing to do. Actually, not even for Miosix OS. Here's why: Adding ARG_NUM_MAX this way requires all targets to define their own ARG_NUM_MAX if they want to support more than 256 args. Right now, there's no such limitation. There already is a standarized way for all targets to tell userspace how much memory can be used by the exec(3) family of functions: ARG_MAX from limits.h or sysconf (_SC_ARG_MAX) from unistd.h must be >= _POSIX_ARG_MAX (4096) ARG_MAX does not define a max number of args. It defines a max number of bytes used for arguments and environment combined. This is what should be tested for in exec(3), if you want to enforce memory usage restrictions. Thanks, Corinna
Hi Corinna, On 27/01/25 14:21, Corinna Vinschen wrote: > Adding ARG_NUM_MAX this way requires all targets to define their own > ARG_NUM_MAX if they want to support more than 256 args. Right now, > there's no such limitation. As far as I can see, right now there is this limitation in newlib. Let's have a look at the current code of one of these functions: https://sourceware.org/git?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/posix/execl.c What I see is a const char *argv[256]; declared on the stack, and a do..while loop copying args into that array with no bound checking. Thus, attempting to pass more than 256 arguments will silently corrupt the stack. Attached please find a test case that when compiled with the address sanitizer fails due to the stack buffer overflow when more than 256 arguments are passed to the execl code above. The purpose of my patch is to: - avoid the stack buffer overflow in all cases - allow targets to override the default 256 arguments limit if they so desire, either lowering to reduce stack use or increasing it if 256 is not enough Let me know if I'm missing something. Best regards, Federico Terraneo
On Jan 27 15:36, Federico Terraneo wrote: > Hi Corinna, > > On 27/01/25 14:21, Corinna Vinschen wrote: > > Adding ARG_NUM_MAX this way requires all targets to define their own > > ARG_NUM_MAX if they want to support more than 256 args. Right now, > > there's no such limitation. > > As far as I can see, right now there is this limitation in newlib. > Let's have a look at the current code of one of these functions: > > https://sourceware.org/git?p=newlib-cygwin.git;a=blob_plain;f=newlib/libc/posix/execl.c > > What I see is a > > const char *argv[256]; > > declared on the stack, and a do..while loop copying args into that array > with no bound checking. Thus, attempting to pass more than 256 arguments > will silently corrupt the stack. > > Attached please find a test case that when compiled with the address > sanitizer fails due to the stack buffer overflow when more than 256 > arguments are passed to the execl code above. > > The purpose of my patch is to: > - avoid the stack buffer overflow in all cases > - allow targets to override the default 256 arguments limit if they so > desire, either lowering to reduce stack use or increasing it if 256 is not > enough Huh, you're right. Apologies, I was not reading all of your patch and was thinking of the Cygwin exec(3), which doesn't have that restriction. I checked the history, the exec functions are using this static stack allocation since the last century, before the CVS repo was created. If we keep your patch, please redefine ARG_NUM_MAX to _ARG_NUM_MAX. But it would still be nice to have a more dynamic solution which doesn't need ARG_NUM_MAX. Thanks, Corinna
On 27/01/25 18:26, Corinna Vinschen wrote: > But it would still be nice to have a more dynamic solution which doesn't > need ARG_NUM_MAX. Hi Corinna, I spent some time into it, and it's more complicated than I wanted, but I have a patch in this direction nonetheless, that replaces the previous patch proposal. The complexity is caused by POSIX.1-2008 which as far as I could tell requires execl* functions to be callable from signal handlers, while malloc isn't. That leaves alloca as the only way I can think of to lift the argument number limitation. The issue is, newlib targets also microcontroller targets where there's no virtual memory and stacks are small (our Miosix OS is one example), thus unbounded stack allocation (or just large stack allocations like the 1024bytes array in the current version of the execl* code) cause stack overflows and undefined behavior. Thus I propose the attached patch that by default uses alloca but allows individual targets to define _EXECL_USE_MALLOC if in their use case the risk of stack overflows and consequent undefined behavior is a more pressing requirement than the corner case of allowing execl* calls from signal handlers. Even with the default alloca implementation, the patched code uses *less* stack than the current code when execl* functions are called with less that 256 arguments, and on platforms where the stack can grow (i.e, when virtual memory is available) it entirely avoids the stack buffer overflow that was present in the current code, so I think it's an overall improvement. Best regards, Federico Terraneo
Hallo Federico Terraneo: > The complexity is caused by POSIX.1-2008 which > [...] > allows individual targets to define _EXECL_USE_MALLOC The reason why _EXECL_USE_MALLOC exists in your patch is well explained in your e-mail, but an e-mail is not the right place for documentation. In order to help future developers looking at the code, I would place the reason as a comment over the first "#ifndef _EXECL_USE_MALLOC". Best regards, rdiez
Hi Federico, On Jan 26 12:14, Federico Terraneo wrote: > Hi, > I propose the following patch to the execl* functions in newlib that does > the following: > - it returns an error if more than the maximum number of arguments are > passed instead of corrupting the stack > - it allows to tweak the maximum number of arguments at the time of > compiling newlib by defining the ARG_NUM_MAX macro > > This patch is originally meant for the Miosix OS to allow reducing the > maximum number of arguments with the intent of reducing the stack usage of > the execl* functions in low memory microcontrollers, but I think it is of > general applicability and can of course be useful also to increase this > value for desktop-class targets. > > Best regards, > Federico Terraneo > > I'm sorry, but I don't think this is the right thing to do. Actually, not even for Miosix OS. Here's why: Adding ARG_NUM_MAX this way requires all targets to define their own ARG_NUM_MAX if they want to support more than 256 args. Right now, there's no such limitation. There already is a standarized way for all targets to tell userspace how much memory can be used by the exec(3) family of functions: ARG_MAX from limits.h or sysconf (_SC_ARG_MAX) from unistd.h must be >= _POSIX_ARG_MAX (4096) ARG_MAX does not define a max number of args. It defines a max number of bytes used for arguments and environment combined. This is what should be tested for in exec(3), if you want to enforce memory usage restrictions. Thanks, Corinna
On Jan 29 09:53, Federico Terraneo wrote: > On 27/01/25 18:26, Corinna Vinschen wrote: > > But it would still be nice to have a more dynamic solution which doesn't > > need ARG_NUM_MAX. > > Hi Corinna, > I spent some time into it, and it's more complicated than I wanted, but I > have a patch in this direction nonetheless, that replaces the previous patch > proposal. > > The complexity is caused by POSIX.1-2008 which as far as I could tell > requires execl* functions to be callable from signal handlers, while malloc > isn't. That leaves alloca as the only way I can think of to lift the > argument number limitation. > > The issue is, newlib targets also microcontroller targets where there's no > virtual memory and stacks are small (our Miosix OS is one example), thus > unbounded stack allocation (or just large stack allocations like the > 1024bytes array in the current version of the execl* code) cause stack > overflows and undefined behavior. > > Thus I propose the attached patch that by default uses alloca but allows > individual targets to define _EXECL_USE_MALLOC if in their use case the risk > of stack overflows and consequent undefined behavior is a more pressing > requirement than the corner case of allowing execl* calls from signal > handlers. > > Even with the default alloca implementation, the patched code uses *less* > stack than the current code when execl* functions are called with less that > 256 arguments, and on platforms where the stack can grow (i.e, when virtual > memory is available) it entirely avoids the stack buffer overflow that was > present in the current code, so I think it's an overall improvement. Your patch looks good. At first I thought it's missing the ARG_MAX check now, but yeah, in fact that should be tested in execve. Can you please add the rational for this change to the commit message? Code comments like R. Diez suggested, would be helpful as well. Also, given this adds a compile time option, it would be helpful to add a matching option to configure.ac and add it to the README file. Thanks, Corinna
On 29/01/25 12:26, Corinna Vinschen wrote: > Your patch looks good. At first I thought it's missing the ARG_MAX > check now, but yeah, in fact that should be tested in execve. > > Can you please add the rational for this change to the commit message? > Code comments like R. Diez suggested, would be helpful as well. > > Also, given this adds a compile time option, it would be helpful to > add a matching option to configure.ac and add it to the README file. > Hi, I tried adding the compile-time option and re-running autoreconf, but I'm hitting an issue. Despite using GNU Autoconf 2.69 and GNU Automake 1.15.1 as stated in the newlib/HOWTO file, running autoreconf in the newlib directory *before* making any changes to configure.ac generates tons of spurious changes to Makefile.in, see attached patch 0002. Any idea what I could be doing wrong, I'm on a Debian 12. In any case, patch 0001 and 0003 look ok, maybe it's possible to just skip patch 0002? Even though I'd be curious to know what's the issue with autoreconf, maybe some of you encountered it already? Best regards, Federico Terraneo
On 31/01/2025 09:53, Federico Terraneo wrote: > On 29/01/25 12:26, Corinna Vinschen wrote: >> Your patch looks good. At first I thought it's missing the ARG_MAX >> check now, but yeah, in fact that should be tested in execve. >> >> Can you please add the rational for this change to the commit message? >> Code comments like R. Diez suggested, would be helpful as well. >> >> Also, given this adds a compile time option, it would be helpful to >> add a matching option to configure.ac and add it to the README file. >> > > Hi, > I tried adding the compile-time option and re-running autoreconf, but I'm hitting an issue. Despite using GNU Autoconf 2.69 and GNU Automake 1.15.1 as stated in the newlib/HOWTO file, running autoreconf in the newlib directory *before* making any changes to configure.ac generates tons of spurious changes to Makefile.in, see attached patch 0002. > Any idea what I could be doing wrong, I'm on a Debian 12. > > In any case, patch 0001 and 0003 look ok, maybe it's possible to just skip patch 0002? Even though I'd be curious to know what's the issue with autoreconf, maybe some of you encountered it already? > > Best regards, > Federico Terraneo You need to use the vanilla autoconf obtainable from the FSF. Debian's version contains changes that generate the extra verbage that you are seeing. R.
On 31/01/25 11:55, Richard Earnshaw (lists) wrote:
> You need to use the vanilla autoconf obtainable from the FSF. Debian's version contains changes that generate the extra verbage that you are seeing.
Hi,
I tried installing autoconf and automake from sources, see the command
list attached below, but I'm still getting the same long list of changes
in newlib/Makefile.in.
However, the first of these changes in the patch is the removal of the line
@HAVE_LIBC_SYS_XTENSA_DIR_TRUE@am__append_64 = libc/sys/xtensa/creat.c
libc/sys/xtensa/isatty.c libc/sys/xtensa/clibrary_init.c
and I don't have the newlib/libc/sys/xtensa directory. I'm making my
patch starting from commit
d52d983e5b69457c706c34097a328a7678107b1a
Author: Corinna Vinschen <corinna@vinschen.de> 2025-01-28 16:50:12
Cygwin: implement posix_close
Is it possible that the directory libc/sys/xtensa got removed, somebody
did not run autoreconf and thus Makefile.in is in a somewhat
inconsistent state in the git repository?
---
How I installed autoconf & automake
wget https://ftpmirror.gnu.org/autoconf/autoconf-2.69.tar.gz
wget https://ftpmirror.gnu.org/automake/automake-1.15.1.tar.gz
mkdir autobins
PFX=`pwd`/autobins
tar xvf autoconf-2.69.tar.gz
cd autoconf-2.69
./configure --prefix=$PFX
make
make install
cd ..
tar xvf automake-1.15.1.tar.gz
cd automake-1.15.1
./configure --prefix=$PFX
make
make install
cd ..
export PATH=`pwd`/autobins/bin:$PATH
On 31/01/2025 12:59, Federico Terraneo wrote: > On 31/01/25 11:55, Richard Earnshaw (lists) wrote: >> You need to use the vanilla autoconf obtainable from the FSF. Debian's version contains changes that generate the extra verbage that you are seeing. > > Hi, > I tried installing autoconf and automake from sources, see the command list attached below, but I'm still getting the same long list of changes in newlib/Makefile.in. > > However, the first of these changes in the patch is the removal of the line > > @HAVE_LIBC_SYS_XTENSA_DIR_TRUE@am__append_64 = libc/sys/xtensa/creat.c libc/sys/xtensa/isatty.c libc/sys/xtensa/clibrary_init.c > > and I don't have the newlib/libc/sys/xtensa directory. I'm making my patch starting from commit > > d52d983e5b69457c706c34097a328a7678107b1a > Author: Corinna Vinschen <corinna@vinschen.de> 2025-01-28 16:50:12 > Cygwin: implement posix_close > > Is it possible that the directory libc/sys/xtensa got removed, somebody did not run autoreconf and thus Makefile.in is in a somewhat inconsistent state in the git repository? > > > --- > How I installed autoconf & automake > > wget https://ftpmirror.gnu.org/autoconf/autoconf-2.69.tar.gz > wget https://ftpmirror.gnu.org/automake/automake-1.15.1.tar.gz > mkdir autobins > PFX=`pwd`/autobins > tar xvf autoconf-2.69.tar.gz > cd autoconf-2.69 > ./configure --prefix=$PFX > make > make install > cd .. > tar xvf automake-1.15.1.tar.gz > cd automake-1.15.1 > ./configure --prefix=$PFX > make > make install > cd .. > export PATH=`pwd`/autobins/bin:$PATH > It can be tricky. Unfortunately, some people don't properly rebuild things after changing the master files. I'd start from a clean repo and try to get to the point where running autoconf is a no-op. Then you'll know that your changes aren't the source of the problems. If you can't then we can dig through the git history to try to identify where things went wrong. R.
On 31/01/25 14:28, Richard Earnshaw (lists) wrote: > It can be tricky. Unfortunately, some people don't properly rebuild things after changing the master files. I'd start from a clean repo and try to get to the point where running autoconf is a no-op. Then you'll know that your changes aren't the source of the problems. If you can't then we can dig through the git history to try to identify where things went wrong. > > R. Found it. It's commit commit 48f1655c955c154b3165692e02aa66699212453c (HEAD) Author: Alexey Lapshin <alexey.lapshin@espressif.com> Date: Fri Aug 30 09:35:24 2024 +0000 newlib: xtensa: remove sys/xtensa. use machine/xtensa and the next commit. They remove the libc/sys/xtensa directory but Makefile.in was not committed. If I autoreconf from the previous commit, autoreconf is a no-op. If I autoreconf after that, I get changes in Makefile.in There apparently is nothing wrong in my autotools installation. I thus propose those two patches. The first just updates newlib/Makefile.in so it is consistent with the current newlib directory. The second is the actual patch I was working on.
> ------------------------------ > *From:* Federico Terraneo <fede.tft@hotmail.it> > *Sent:* Friday, January 31, 2025 10:02 AM > *To:* Richard Earnshaw (lists) <Richard.Earnshaw@arm.com>; > newlib@sourceware.org <newlib@sourceware.org> > *Subject:* Re: [PATCH] Improve execl* functions > > > ... > > There apparently is nothing wrong in my autotools installation. > I thus propose those two patches. > > The first just updates newlib/Makefile.in so it is consistent with the > current newlib directory. > > The second is the actual patch I was working on. > Federico: Two minor things. First, typo in repeated comment "override the default an use malloc() */", change "an" to "and". Second, suggest gating the associated include files as the code is gated (marked as a patch to the result of your patch): +#ifdef _EXECL_USE_MALLOC #include <errno.h> #include <stdlib.h> +#else #include <alloca.h> +#endif Craig
Hi, updated patch 0002.
Hi Federico, On Jan 31 17:42, Federico Terraneo wrote: > Hi, > updated patch 0002. Both patches pushed. Thanks, Corinna > From a13bf0a67ab85138cd31a6673494f30284fb9b60 Mon Sep 17 00:00:00 2001 > From: Terraneo Federico <fede.tft@miosix.org> > Date: Tue, 28 Jan 2025 17:19:18 +0100 > Subject: [PATCH 2/2] Lift 256 arg limit in execl, execle and execlp, add > --enable-newlib-use-malloc-in-execl option > > The previous version of these functions allocated a 256 entry array and copied > arguments in that array with no bound checking. That implementation always > occupied 1024 bytes of stack for the array even in the common case in which the > number of passed arguments is far less than 256, risking stack overflows in > environments with small stacks, and caused a stack buffer overflow if called > with more than 256 arguments. > > The improved implementation counts the actual number of passed arguments and > allocates a suitable buffer. The default implementation uses alloca to allocate > the buffer to satisfy the POSIX.1-2008 requirement that execl and execle should > be callable from signal handlers, but it is possible to override this behavior > and use malloc for targets where the risk of stack overflow due to unbounded > stack allocations is a more pressing requirement than the corner case of > allowing execl calls from signal handlers. > --- > newlib/README | 8 ++++++++ > newlib/configure | 19 +++++++++++++++++++ > newlib/configure.ac | 13 +++++++++++++ > newlib/libc/posix/execl.c | 36 ++++++++++++++++++++++++++++++++++-- > newlib/libc/posix/execle.c | 36 ++++++++++++++++++++++++++++++++++-- > newlib/libc/posix/execlp.c | 31 +++++++++++++++++++++++++++++-- > newlib/newlib.hin | 3 +++ > 7 files changed, 140 insertions(+), 6 deletions(-)
From 9d06b54b1f2a2957bc40af10c6dc09867662325f Mon Sep 17 00:00:00 2001 From: Terraneo Federico <fede.tft@miosix.org> Date: Sat, 25 Jan 2025 11:51:15 +0100 Subject: [PATCH] Fix stack overflow in exec*; add ARG_NUM_MAX --- newlib/libc/posix/execl.c | 19 +++++++++++++++---- newlib/libc/posix/execle.c | 17 ++++++++++++++--- newlib/libc/posix/execlp.c | 19 +++++++++++++++---- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/newlib/libc/posix/execl.c b/newlib/libc/posix/execl.c index c3b4e55bd..044205cc6 100644 --- a/newlib/libc/posix/execl.c +++ b/newlib/libc/posix/execl.c @@ -7,6 +7,7 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> /* Only deal with a pointer to environ, to work around subtle bugs with shared libraries and/or small data systems where the user declares his own @@ -16,6 +17,10 @@ static char ***p_environ = &environ; #include <stdarg.h> +#ifndef ARG_NUM_MAX +#define ARG_NUM_MAX 256 +#endif + int execl (const char *path, const char *arg0, ...) @@ -24,14 +29,20 @@ execl (const char *path, { int i; va_list args; - const char *argv[256]; + const char *argv[ARG_NUM_MAX]; va_start (args, arg0); argv[0] = arg0; i = 1; - do - argv[i] = va_arg (args, const char *); - while (argv[i++] != NULL); + do { + if(i>=ARG_NUM_MAX) + { + va_end (args); + errno=E2BIG; + return -1; + } + argv[i] = va_arg (args, const char *); + } while (argv[i++] != NULL); va_end (args); return _execve (path, (char * const *) argv, *p_environ); diff --git a/newlib/libc/posix/execle.c b/newlib/libc/posix/execle.c index 34f0ea373..7a25ae8ef 100644 --- a/newlib/libc/posix/execle.c +++ b/newlib/libc/posix/execle.c @@ -7,10 +7,15 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> #include <stdarg.h> +#ifndef ARG_NUM_MAX +#define ARG_NUM_MAX 256 +#endif + int execle (const char *path, const char *arg0, ...) @@ -20,14 +25,20 @@ execle (const char *path, int i; va_list args; const char * const *envp; - const char *argv[256]; + const char *argv[ARG_NUM_MAX]; va_start (args, arg0); argv[0] = arg0; i = 1; - do + do { + if(i>=ARG_NUM_MAX) + { + va_end (args); + errno=E2BIG; + return -1; + } argv[i] = va_arg (args, const char *); - while (argv[i++] != NULL); + } while (argv[i++] != NULL); envp = va_arg (args, const char * const *); va_end (args); diff --git a/newlib/libc/posix/execlp.c b/newlib/libc/posix/execlp.c index b845c88c5..a437a56f0 100644 --- a/newlib/libc/posix/execlp.c +++ b/newlib/libc/posix/execlp.c @@ -7,10 +7,15 @@ #include <_ansi.h> #include <unistd.h> +#include <errno.h> #include <stdarg.h> +#ifndef ARG_NUM_MAX +#define ARG_NUM_MAX 256 +#endif + int execlp (const char *path, const char *arg0, ...) @@ -19,14 +24,20 @@ execlp (const char *path, { int i; va_list args; - const char *argv[256]; + const char *argv[ARG_NUM_MAX]; va_start (args, arg0); argv[0] = arg0; i = 1; - do - argv[i] = va_arg (args, const char *); - while (argv[i++] != NULL); + do { + if(i>=ARG_NUM_MAX) + { + va_end (args); + errno=E2BIG; + return -1; + } + argv[i] = va_arg (args, const char *); + } while (argv[i++] != NULL); va_end (args); return execvp (path, (char * const *) argv); -- 2.39.5