From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.1 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 26967 invoked from network); 29 Jan 2022 13:43:48 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 29 Jan 2022 13:43:48 -0000 Received: (qmail 16015 invoked by uid 550); 29 Jan 2022 13:43:45 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 15979 invoked from network); 29 Jan 2022 13:43:44 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=zhasha.com; s=wirbelwind; t=1643463809; bh=PyaknFfk/Z9WAUWhzGUGVexFlIrdyGMbGGkn0JthTRU=; h=Date:From:To:Subject; b=YLDaw/YqgfJVVKcmhPepSE/mDZ/Zj+lAgwAmZlkrT7fXGhf/fdvgGpzmVfPIBujAU CmMr3enCdlauK0mfjthLnJsBTFPIg0p+VbTWjFPc4gsVz0hkAB3d3WmYFkPCZMjP3m Phz4HdltNyQ7CiOH5U+clD0yUYpt9tuSm/GClZVQ= Date: Sat, 29 Jan 2022 14:43:28 +0100 From: Joakim Sindholt To: musl@lists.openwall.com Message-ID: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="SrPL0tT5ZOPFHm7E" Content-Disposition: inline Subject: [musl] [PATCH] pack posix_spawn file actions into extraneous padding --SrPL0tT5ZOPFHm7E Content-Type: text/plain; charset=us-ascii Content-Disposition: inline I've written this patch that packs posix_spawn file actions into the posix_spawn_file_actions struct padding. I'm not sure if the way I've split the code up is desirable. Maybe the faexpand() function needs a different name and to be in its own .c file; or maybe a different approach is preferable. I've only lightly tested it but it seems to work just fine as-is. The whole explanation of how it works can be found in the patch file. --SrPL0tT5ZOPFHm7E Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-pack-posix_spawn-file-actions-into-extraneous-paddin.patch" >From 94c650b7d6df4ee6592f6d44977b461fc19df4f1 Mon Sep 17 00:00:00 2001 From: Joakim Sindholt Date: Sat, 29 Jan 2022 14:05:23 +0100 Subject: [PATCH] pack posix_spawn file actions into extraneous padding The posix_spawn_file_actions struct contains 2 int paddings of 2 and 16 ints respectively, making them ideal for packing file actions into. The encoding used is simple: __pad0[0] contains a count and __pad contains the actuals elements. The most important actions, dup2 and close, are encoded as 2 and 1 int respectively. If the first element of a sequence is negative, then it's a close and processing stops there. If the second element of a sequence is positive, then it's a dup2 and processing stops there. If the second element is negative, however, then it contains a command and requires per-command specific processing. In total there are 5 file actions. Their encodings are: close: -fd-1 dup2: fd, srcfd fchdir: fd, -FDOP_FCHDIR chdir: 0, -FDOP_CHDIR open: fd, -FDOP_OPEN, flags, mode Some file actions need a string. When a string is needed it's allocated on the tail end of a new file_actions struct. When a string is needed or an action requires more space than is available then a new struct is allocated and __actions is set to point to it. All the elements of the first file_actions struct is copies to the new one. The new file_actions struct, being the second in the list, always points to the last file_actions struct in the list. From then on the structs all point back in reverse order until they reach the second one again, creating a loop. This rube-goldberg machine exists because there's nowhere to store a last pointer for appending new actions efficiently. As a result of this backwards allocation, the actions requiring strings aren't stored in the same struct as the strings themselves. Rather they get stored in the logically previous struct when reading. This packing is far from optimal but it is very simple and it allows for up to 16 close, 8 dup2, or any combination of them, without needing to call malloc. At worst it calls malloc as much as before. --- src/process/fdop.h | 24 +++++-- src/process/posix_spawn.c | 67 ++++++++++++++----- .../posix_spawn_file_actions_addchdir.c | 13 ++-- .../posix_spawn_file_actions_addclose.c | 10 +-- .../posix_spawn_file_actions_adddup2.c | 12 ++-- .../posix_spawn_file_actions_addfchdir.c | 11 ++- .../posix_spawn_file_actions_addopen.c | 18 +++-- .../posix_spawn_file_actions_destroy.c | 15 +++-- src/process/posix_spawn_file_actions_init.c | 1 + 9 files changed, 102 insertions(+), 69 deletions(-) diff --git a/src/process/fdop.h b/src/process/fdop.h index 7cf733b2..4b25363c 100644 --- a/src/process/fdop.h +++ b/src/process/fdop.h @@ -4,14 +4,24 @@ #define FDOP_CHDIR 4 #define FDOP_FCHDIR 5 -struct fdop { - struct fdop *next, *prev; - int cmd, fd, srcfd, oflag; - mode_t mode; - char path[]; -}; - #define malloc __libc_malloc #define calloc __libc_calloc #define realloc undef #define free __libc_free + +static inline int faexpand(posix_spawn_file_actions_t *fa, size_t str) +{ + posix_spawn_file_actions_t *n, *blk; + blk = malloc(sizeof(*blk)+str); + if (!blk) return -1; + *blk = *fa; + if (fa->__actions) { + n = fa->__actions; + blk->__actions = n->__actions; + n->__actions = blk; + } else + blk->__actions = blk; + fa->__actions = blk; + fa->__pad0[0] = 0; + return 0; +} diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c index 728551b3..42bd4adb 100644 --- a/src/process/posix_spawn.c +++ b/src/process/posix_spawn.c @@ -81,33 +81,65 @@ static int child(void *args_vp) (ret=__syscall(SYS_setuid, __syscall(SYS_getuid))) ) goto fail; - if (fa && fa->__actions) { - struct fdop *op; - int fd; - for (op = fa->__actions; op->next; op = op->next); - for (; op; op = op->prev) { + if (fa) { + const posix_spawn_file_actions_t *first; + const char *path; + int fd, srcfd, flags, cmd, i = 0; + mode_t mode; + + first = fa; + if (fa->__actions) { + fa = fa->__actions; + fa = fa->__actions; + } + while (fa) { + if (i == fa->__pad0[0]) { + if (fa == first) + break; + path = (char *)(fa+1); + fa = fa==first->__actions ? first : fa->__actions; + i = 0; + continue; + } + fd = fa->__pad[i++]; + if (fd < 0) { + fd = -fd-1; + cmd = FDOP_CLOSE; + } else switch ((cmd = -fa->__pad[i++])) { + case FDOP_OPEN: + flags = fa->__pad[i++]; + mode = (mode_t)fa->__pad[i++]; + break; + case FDOP_CHDIR: + fd = -1; + break; + case FDOP_FCHDIR: + break; + default: + srcfd = -cmd; + cmd = FDOP_DUP2; + } /* It's possible that a file operation would clobber * the pipe fd used for synchronizing with the * parent. To avoid that, we dup the pipe onto * an unoccupied fd. */ - if (op->fd == p) { + if (fd == p) { ret = __syscall(SYS_dup, p); if (ret < 0) goto fail; __syscall(SYS_close, p); p = ret; } - switch(op->cmd) { + switch(cmd) { case FDOP_CLOSE: - __syscall(SYS_close, op->fd); + __syscall(SYS_close, fd); break; case FDOP_DUP2: - fd = op->srcfd; - if (fd == p) { + if (srcfd == p) { ret = -EBADF; goto fail; } - if (fd != op->fd) { - if ((ret=__sys_dup2(fd, op->fd))<0) + if (srcfd != fd) { + if ((ret=__sys_dup2(srcfd, fd))<0) goto fail; } else { ret = __syscall(SYS_fcntl, fd, F_GETFD); @@ -118,20 +150,21 @@ static int child(void *args_vp) } break; case FDOP_OPEN: - fd = __sys_open(op->path, op->oflag, op->mode); + srcfd = fd; + fd = __sys_open(path, flags, mode); if ((ret=fd) < 0) goto fail; - if (fd != op->fd) { - if ((ret=__sys_dup2(fd, op->fd))<0) + if (fd != srcfd) { + if ((ret=__sys_dup2(fd, srcfd))<0) goto fail; __syscall(SYS_close, fd); } break; case FDOP_CHDIR: - ret = __syscall(SYS_chdir, op->path); + ret = __syscall(SYS_chdir, path); if (ret<0) goto fail; break; case FDOP_FCHDIR: - ret = __syscall(SYS_fchdir, op->fd); + ret = __syscall(SYS_fchdir, fd); if (ret<0) goto fail; break; } diff --git a/src/process/posix_spawn_file_actions_addchdir.c b/src/process/posix_spawn_file_actions_addchdir.c index 7f2590ae..1aac8198 100644 --- a/src/process/posix_spawn_file_actions_addchdir.c +++ b/src/process/posix_spawn_file_actions_addchdir.c @@ -6,13 +6,10 @@ int posix_spawn_file_actions_addchdir_np(posix_spawn_file_actions_t *restrict fa, const char *restrict path) { - struct fdop *op = malloc(sizeof *op + strlen(path) + 1); - if (!op) return ENOMEM; - op->cmd = FDOP_CHDIR; - op->fd = -1; - strcpy(op->path, path); - if ((op->next = fa->__actions)) op->next->prev = op; - op->prev = 0; - fa->__actions = op; + size_t len = strlen(path); + if (faexpand(fa, len + 1) != 0) return ENOMEM; + fa->__pad[fa->__pad0[0]++] = 0; + fa->__pad[fa->__pad0[0]++] = -FDOP_CHDIR; + memcpy((posix_spawn_file_actions_t *)fa->__actions + 1, path, len + 1); return 0; } diff --git a/src/process/posix_spawn_file_actions_addclose.c b/src/process/posix_spawn_file_actions_addclose.c index 0c2ef8fa..c6942fc0 100644 --- a/src/process/posix_spawn_file_actions_addclose.c +++ b/src/process/posix_spawn_file_actions_addclose.c @@ -6,12 +6,8 @@ int posix_spawn_file_actions_addclose(posix_spawn_file_actions_t *fa, int fd) { if (fd < 0) return EBADF; - struct fdop *op = malloc(sizeof *op); - if (!op) return ENOMEM; - op->cmd = FDOP_CLOSE; - op->fd = fd; - if ((op->next = fa->__actions)) op->next->prev = op; - op->prev = 0; - fa->__actions = op; + if (sizeof(fa->__pad)/sizeof(*fa->__pad) - fa->__pad0[0] < 1) + if (faexpand(fa, 0) != 0) return ENOMEM; + fa->__pad[fa->__pad0[0]++] = -fd-1; return 0; } diff --git a/src/process/posix_spawn_file_actions_adddup2.c b/src/process/posix_spawn_file_actions_adddup2.c index addca4d4..b2945f56 100644 --- a/src/process/posix_spawn_file_actions_adddup2.c +++ b/src/process/posix_spawn_file_actions_adddup2.c @@ -6,13 +6,9 @@ int posix_spawn_file_actions_adddup2(posix_spawn_file_actions_t *fa, int srcfd, int fd) { if (srcfd < 0 || fd < 0) return EBADF; - struct fdop *op = malloc(sizeof *op); - if (!op) return ENOMEM; - op->cmd = FDOP_DUP2; - op->srcfd = srcfd; - op->fd = fd; - if ((op->next = fa->__actions)) op->next->prev = op; - op->prev = 0; - fa->__actions = op; + if (sizeof(fa->__pad)/sizeof(*fa->__pad) - fa->__pad0[0] < 2) + if (faexpand(fa, 0) != 0) return ENOMEM; + fa->__pad[fa->__pad0[0]++] = fd; + fa->__pad[fa->__pad0[0]++] = srcfd; return 0; } diff --git a/src/process/posix_spawn_file_actions_addfchdir.c b/src/process/posix_spawn_file_actions_addfchdir.c index e89ede8c..a62a35fa 100644 --- a/src/process/posix_spawn_file_actions_addfchdir.c +++ b/src/process/posix_spawn_file_actions_addfchdir.c @@ -7,12 +7,9 @@ int posix_spawn_file_actions_addfchdir_np(posix_spawn_file_actions_t *fa, int fd) { if (fd < 0) return EBADF; - struct fdop *op = malloc(sizeof *op); - if (!op) return ENOMEM; - op->cmd = FDOP_FCHDIR; - op->fd = fd; - if ((op->next = fa->__actions)) op->next->prev = op; - op->prev = 0; - fa->__actions = op; + if (sizeof(fa->__pad)/sizeof(*fa->__pad) - fa->__pad0[0] < 2) + if (faexpand(fa, 0) != 0) return ENOMEM; + fa->__pad[fa->__pad0[0]++] = fd; + fa->__pad[fa->__pad0[0]++] = -FDOP_FCHDIR; return 0; } diff --git a/src/process/posix_spawn_file_actions_addopen.c b/src/process/posix_spawn_file_actions_addopen.c index 82bbcec9..1c419396 100644 --- a/src/process/posix_spawn_file_actions_addopen.c +++ b/src/process/posix_spawn_file_actions_addopen.c @@ -6,16 +6,14 @@ int posix_spawn_file_actions_addopen(posix_spawn_file_actions_t *restrict fa, int fd, const char *restrict path, int flags, mode_t mode) { + size_t len; if (fd < 0) return EBADF; - struct fdop *op = malloc(sizeof *op + strlen(path) + 1); - if (!op) return ENOMEM; - op->cmd = FDOP_OPEN; - op->fd = fd; - op->oflag = flags; - op->mode = mode; - strcpy(op->path, path); - if ((op->next = fa->__actions)) op->next->prev = op; - op->prev = 0; - fa->__actions = op; + len = strlen(path); + if (faexpand(fa, len + 1) != 0) return ENOMEM; + fa->__pad[fa->__pad0[0]++] = fd; + fa->__pad[fa->__pad0[0]++] = -FDOP_OPEN; + fa->__pad[fa->__pad0[0]++] = flags; + fa->__pad[fa->__pad0[0]++] = mode; + memcpy((posix_spawn_file_actions_t *)fa->__actions + 1, path, len + 1); return 0; } diff --git a/src/process/posix_spawn_file_actions_destroy.c b/src/process/posix_spawn_file_actions_destroy.c index 3251babb..514d2344 100644 --- a/src/process/posix_spawn_file_actions_destroy.c +++ b/src/process/posix_spawn_file_actions_destroy.c @@ -4,11 +4,16 @@ int posix_spawn_file_actions_destroy(posix_spawn_file_actions_t *fa) { - struct fdop *op = fa->__actions, *next; - while (op) { - next = op->next; - free(op); - op = next; + posix_spawn_file_actions_t *next, *p; + if (fa->__actions) { + p = fa->__actions; + p = p->__actions; + while (p != fa->__actions) { + next = p->__actions; + free(p); + p = next; + } + free(fa->__actions); } return 0; } diff --git a/src/process/posix_spawn_file_actions_init.c b/src/process/posix_spawn_file_actions_init.c index 89d5e127..49baf889 100644 --- a/src/process/posix_spawn_file_actions_init.c +++ b/src/process/posix_spawn_file_actions_init.c @@ -2,6 +2,7 @@ int posix_spawn_file_actions_init(posix_spawn_file_actions_t *fa) { + fa->__pad0[0] = 0; fa->__actions = 0; return 0; } -- 2.26.3 --SrPL0tT5ZOPFHm7E--