Hi! Here's an alternative 4/4 patch fixing the same bug as in the previous 4/4 "wordexp: step past we_offs words when freeing" patch but in a cleaner way (IMHO). It also optimizes the memory allocations in the process, killing the need to strdup all expanded words. It doesn't seem like I need to list the new wordexp2.h file anywhere in the build system, that surprised me, but I simply can't find other internal header files in any infrastructure list... Cheers, Peter 2012-09-14 Peter Rosin * libc/posix/wordfree.c (wordfree): The wrong words are freed when WRDE_DOOFFS is in use. Restructure the code so that the memory needed to be freed is instead kept in an internal linked list... * libc/posix/wordexp2.h: ...as defined here... * libc/posix/wordfree.c (wordfree): ...and build this internal linked list here, avoiding wasteful strdup calls in the process. Index: newlib/libc/posix/wordexp.c =================================================================== --- newlib.orig/libc/posix/wordexp.c +++ newlib/libc/posix/wordexp.c @@ -18,8 +18,10 @@ #include #include #include +#include #include +#include "wordexp2.h" #define MAXLINELEN 500 @@ -41,9 +43,9 @@ wordexp(const char *words, wordexp_t *pw int fd[2]; int fd_err[2]; int err = WRDE_NOSPACE; - char **wordv; - char *ewords = NULL; + ext_wordv_t *wordv = NULL; char *eword; + struct ewords_entry *entry; if (pwordexp == NULL) { @@ -63,10 +65,14 @@ wordexp(const char *words, wordexp_t *pw { offs = pwordexp->we_offs; - wordv = (char **)realloc(pwordexp->we_wordv, (pwordexp->we_wordc + offs + 1) * sizeof(char *)); + if (pwordexp->we_wordv) + wordv = WE_WORDV_TO_EXT_WORDV(pwordexp->we_wordv); + wordv = (ext_wordv_t *)realloc(wordv, sizeof(ext_wordv_t) + (offs + pwordexp->we_wordc) * sizeof(char *)); if (!wordv) return err; - pwordexp->we_wordv = wordv; + if (!pwordexp->we_wordv) + SLIST_INIT(&wordv->list); + pwordexp->we_wordv = wordv->we_wordv; for (i = 0; i < offs; i++) pwordexp->we_wordv[i] = NULL; @@ -142,11 +148,14 @@ wordexp(const char *words, wordexp_t *pw num_words = atoi(tmp); - wordv = (char **)realloc(pwordexp->we_wordv, - (pwordexp->we_wordc + num_words + offs + 1) * sizeof(char *)); + if (pwordexp->we_wordv) + wordv = we_wordv_to_ext_wordv(pwordexp->we_wordv); + wordv = (ext_wordv_t *)realloc(wordv, sizeof(ext_wordv_t) + (offs + pwordexp->we_wordc + num_words) * sizeof(char *)); if (!wordv) - goto cleanup; - pwordexp->we_wordv = wordv; + return err; + if (!pwordexp->we_wordv) + SLIST_INIT(&wordv->list); + pwordexp->we_wordv = wordv->we_wordv; /* Get number of bytes required for storage of all num_words words. */ if (!fgets(tmp, MAXLINELEN, f)) @@ -157,36 +166,32 @@ wordexp(const char *words, wordexp_t *pw num_bytes = atoi(tmp); - /* Get expansion from the shell output. */ - if (!(ewords = (char *)malloc(num_bytes + num_words + 1))) + if (!(entry = (struct ewords_entry *)malloc(sizeof(struct ewords_entry) + num_bytes + num_words))) goto cleanup; - if (!fread(ewords, 1, num_bytes + num_words, f)) + SLIST_INSERT_HEAD(&wordv->list, entry, next); + + /* Get expansion from the shell output. */ + if (!fread(entry->ewords, 1, num_bytes + num_words, f)) goto cleanup; - ewords[num_bytes + num_words] = 0; + entry->ewords[num_bytes + num_words] = 0; /* Store each entry in pwordexp's we_wordv vector. */ - eword = ewords; - for(i = 0; i < num_words; i++) + eword = entry->ewords; + for(i = 0; i < num_words; i++, eword = iter) { - if (eword && (iter = strchr(eword, '\n'))) - *iter = '\0'; - - if (!eword || - !(pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = strdup(eword))) - { - pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL; - pwordexp->we_wordc += i; - goto cleanup; - } - eword = iter ? iter + 1 : iter; + if (!eword) + break; + pwordexp->we_wordv[offs + pwordexp->we_wordc + i] = eword; + if ((iter = strchr(eword, '\n'))) + *iter++ = '\0'; } - pwordexp->we_wordv[pwordexp->we_wordc + offs + i] = NULL; + pwordexp->we_wordv[offs + pwordexp->we_wordc + i] = NULL; pwordexp->we_wordc += num_words; - err = WRDE_SUCCESS; + if (i == num_words) + err = WRDE_SUCCESS; cleanup: - free(ewords); if (f) fclose(f); else Index: newlib/libc/posix/wordexp2.h =================================================================== --- /dev/null +++ newlib/libc/posix/wordexp2.h @@ -0,0 +1,21 @@ +/* Copyright (C) 2012 by Peter Rosin. All rights reserved. + * + * Permission to use, copy, modify, and distribute this software + * is freely granted, provided that this notice is preserved. + */ +#ifndef _WORDEXP2_H_ + +struct ewords_entry { + SLIST_ENTRY(ewords_entry) next; + char ewords[1]; +}; + +typedef struct { + SLIST_HEAD(ewords_head, ewords_entry) list; + char *we_wordv[1]; +} ext_wordv_t; + +#define WE_WORDV_TO_EXT_WORDV(wordv) \ + ((ext_wordv_t *)((void *)(wordv) - offsetof(ext_wordv_t, we_wordv))) + +#endif /* !_WORDEXP2_H_ */ Index: newlib/libc/posix/wordfree.c =================================================================== --- newlib.orig/libc/posix/wordfree.c +++ newlib/libc/posix/wordfree.c @@ -18,13 +18,15 @@ #include #include #include +#include #include +#include "wordexp2.h" void wordfree(wordexp_t *pwordexp) { - int i; + ext_wordv_t *wordv; if (pwordexp == NULL) return; @@ -32,10 +34,14 @@ wordfree(wordexp_t *pwordexp) if (pwordexp->we_wordv == NULL) return; - for(i = 0; i < pwordexp->we_wordc; i++) - free(pwordexp->we_wordv[i]); + wordv = WE_WORDV_TO_EXT_WORDV(pwordexp->we_wordv); + while (!SLIST_EMPTY(&wordv->list)) { + struct ewords_entry *entry = SLIST_FIRST(&wordv->list); + SLIST_REMOVE_HEAD(&wordv->list, next); + free(entry); + } - free(pwordexp->we_wordv); + free(wordv); pwordexp->we_wordv = NULL; }