Source-Changes-HG archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
[src/trunk]: src/usr.bin/make make(1): fix double-free bug in -DCLEANUP mode ...
details: https://anonhg.NetBSD.org/src/rev/33f55d4f5fd6
branches: trunk
changeset: 955642:33f55d4f5fd6
user: rillig <rillig%NetBSD.org@localhost>
date: Mon Oct 05 19:24:29 2020 +0000
description:
make(1): fix double-free bug in -DCLEANUP mode (since 2020-10-02)
The bug had been introduced with dir.c 1.155 on 2020-10-02 22:20:25. In
that commit, openDirectories was replaced with a combination of a list
with a hash table, for more efficient lookup by name.
Upon cleanup, OpenDirs_Done is called, which in turn called
Dir_ClearPath. Dir_ClearPath takes full ownership of the given list and
empties it. This was no problem before since afterwards the list was
empty and calling Lst_Free just frees the remaining list pointer.
With OpenDirs, this list was combined with a hash table, and the hash
table contains the list nodes, assuming that the OpenDirs functions have
full ownership of both the list and the hash table. This assumption was
generally correct, except for the one moment during cleanup where full
ownership of the list was passed to Dir_ClearPath, while the hash table
still contained pointers to the (now freed) list nodes. This by itself
was not a problem since the hash table would be freed afterwards. But
as part of Dir_ClearPath, OpenDirs_Remove was called, which looked up
the freed directory by name and now found the freed list node, trying to
free it again. Boom.
Fixed by replacing the call to Dir_ClearPath with code that only frees
the directories, without giving up control over the list.
diffstat:
usr.bin/make/Makefile | 9 +-
usr.bin/make/arch.c | 157 +-
usr.bin/make/compat.c | 89 +-
usr.bin/make/cond.c | 21 +-
usr.bin/make/dir.c | 88 +-
usr.bin/make/enum.c | 14 +-
usr.bin/make/for.c | 19 +-
usr.bin/make/hash.c | 194 +-
usr.bin/make/job.c | 26 +-
usr.bin/make/main.c | 134 +-
usr.bin/make/make.h | 45 +-
usr.bin/make/make_malloc.c | 7 +-
usr.bin/make/nonints.h | 28 +-
usr.bin/make/parse.c | 1551 ++++++++---------
usr.bin/make/str.c | 44 +-
usr.bin/make/suff.c | 8 +-
usr.bin/make/targ.c | 11 +-
usr.bin/make/trace.c | 7 +-
usr.bin/make/unit-tests/Makefile | 13 +-
usr.bin/make/unit-tests/directive-export-literal.exp | 1 -
usr.bin/make/unit-tests/directive-export-literal.mk | 11 +-
usr.bin/make/unit-tests/directive-ifndef.exp | 1 -
usr.bin/make/unit-tests/directive-ifndef.mk | 22 +-
usr.bin/make/unit-tests/directive-ifnmake.exp | 2 -
usr.bin/make/unit-tests/directive-ifnmake.mk | 22 +-
usr.bin/make/unit-tests/make-exported.exp | 1 +
usr.bin/make/unit-tests/make-exported.mk | 21 +-
usr.bin/make/unit-tests/opt-debug-file.mk | 32 +-
usr.bin/make/unit-tests/opt-debug-for.exp | 21 -
usr.bin/make/unit-tests/opt-debug-for.mk | 21 +-
usr.bin/make/unit-tests/opt-debug-jobs.exp | 24 -
usr.bin/make/unit-tests/opt-debug-jobs.mk | 25 +-
usr.bin/make/unit-tests/opt-debug-lint.exp | 4 -
usr.bin/make/unit-tests/opt-debug-lint.mk | 14 +-
usr.bin/make/unit-tests/opt-debug-loud.exp | 2 -
usr.bin/make/unit-tests/opt-debug-loud.mk | 19 +-
usr.bin/make/unit-tests/opt-debug.exp | 3 -
usr.bin/make/unit-tests/opt-debug.mk | 12 +-
usr.bin/make/unit-tests/var-op-append.mk | 31 +-
usr.bin/make/unit-tests/varname-dot-curdir.mk | 21 +-
usr.bin/make/util.c | 5 +-
usr.bin/make/var.c | 80 +-
42 files changed, 1257 insertions(+), 1603 deletions(-)
diffs (truncated from 4523 to 300 lines):
diff -r 3ea5597e6fdf -r 33f55d4f5fd6 usr.bin/make/Makefile
--- a/usr.bin/make/Makefile Mon Oct 05 18:29:20 2020 +0000
+++ b/usr.bin/make/Makefile Mon Oct 05 19:24:29 2020 +0000
@@ -1,4 +1,4 @@
-# $NetBSD: Makefile,v 1.100 2020/10/05 15:11:37 rillig Exp $
+# $NetBSD: Makefile,v 1.101 2020/10/05 19:24:29 rillig Exp $
# @(#)Makefile 5.2 (Berkeley) 12/28/90
PROG= make
@@ -189,10 +189,3 @@
rm -f *.gcov *.gcda
.endif
${.MAKE} test
-
-# Just out of curiosity, during development.
-.SUFFIXES: .cpre .casm
-.c.cpre:
- ${COMPILE.c:S,^-c$,-E,} ${.IMPSRC} -o ${.TARGET}
-.c.casm:
- ${COMPILE.c:S,^-c$,-S,} ${.IMPSRC} -o ${.TARGET}
diff -r 3ea5597e6fdf -r 33f55d4f5fd6 usr.bin/make/arch.c
--- a/usr.bin/make/arch.c Mon Oct 05 18:29:20 2020 +0000
+++ b/usr.bin/make/arch.c Mon Oct 05 19:24:29 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: arch.c,v 1.130 2020/10/03 21:52:50 rillig Exp $ */
+/* $NetBSD: arch.c,v 1.131 2020/10/05 19:24:29 rillig Exp $ */
/*
* Copyright (c) 1988, 1989, 1990, 1993
@@ -123,14 +123,18 @@
#include <sys/param.h>
#include <ar.h>
+#include <ctype.h>
+#include <stdio.h>
+#include <stdlib.h>
#include <utime.h>
#include "make.h"
+#include "hash.h"
#include "dir.h"
#include "config.h"
/* "@(#)arch.c 8.2 (Berkeley) 1/2/94" */
-MAKE_RCSID("$NetBSD: arch.c,v 1.130 2020/10/03 21:52:50 rillig Exp $");
+MAKE_RCSID("$NetBSD: arch.c,v 1.131 2020/10/05 19:24:29 rillig Exp $");
#ifdef TARGET_MACHINE
#undef MAKE_MACHINE
@@ -147,11 +151,11 @@
static ArchList *archives; /* The archives we've already examined */
typedef struct Arch {
- char *name; /* Name of archive */
- Hash_Table members; /* All the members of the archive described
- * by <name, struct ar_hdr *> key/value pairs */
- char *fnametab; /* Extended name table strings */
- size_t fnamesize; /* Size of the string table */
+ char *name; /* Name of archive */
+ Hash_Table members; /* All the members of the archive described
+ * by <name, struct ar_hdr *> key/value pairs */
+ char *fnametab; /* Extended name table strings */
+ size_t fnamesize; /* Size of the string table */
} Arch;
static FILE *ArchFindMember(const char *, const char *,
@@ -166,8 +170,8 @@
ArchFree(void *ap)
{
Arch *a = (Arch *)ap;
- Hash_Search search;
- Hash_Entry *entry;
+ Hash_Search search;
+ Hash_Entry *entry;
/* Free memory from hash entries */
for (entry = Hash_EnumFirst(&a->members, &search);
@@ -254,7 +258,7 @@
* place and skip to the end of it (either white-space or
* a close paren).
*/
- Boolean doSubst = FALSE; /* TRUE if need to substitute in memName */
+ Boolean doSubst = FALSE; /* TRUE if need to substitute in memName */
while (*cp != '\0' && *cp != ')' && ch_isspace(*cp)) {
cp++;
@@ -266,7 +270,7 @@
* Variable spec, so call the Var module to parse the puppy
* so we can safely advance beyond it...
*/
- void *freeIt;
+ void *freeIt;
const char *result;
Boolean isError;
const char *nested_p = cp;
@@ -320,11 +324,11 @@
* later.
*/
if (doSubst) {
- char *buf;
- char *sacrifice;
- char *oldMemName = memName;
+ char *buf;
+ char *sacrifice;
+ char *oldMemName = memName;
- (void)Var_Subst(memName, ctxt, VARE_UNDEFERR|VARE_WANTRES,
+ (void)Var_Subst(memName, ctxt, VARE_UNDEFERR | VARE_WANTRES,
&memName);
/* TODO: handle errors */
@@ -409,9 +413,15 @@
free(libName);
}
- cp++; /* skip the ')' */
- /* We promised that linePtr would be set up at the next non-space. */
- pp_skip_whitespace(&cp);
+ /*
+ * We promised the pointer would be set up at the next non-space, so
+ * we must advance cp there before setting *linePtr... (note that on
+ * entrance to the loop, cp is guaranteed to point at a ')')
+ */
+ do {
+ cp++;
+ } while (*cp != '\0' && ch_isspace(*cp));
+
*linePtr = cp;
return TRUE;
}
@@ -439,15 +449,15 @@
static struct ar_hdr *
ArchStatMember(const char *archive, const char *member, Boolean hash)
{
-#define AR_MAX_NAME_LEN (sizeof(arh.ar_name) - 1)
- FILE *arch; /* Stream to archive */
- size_t size; /* Size of archive member */
- char magic[SARMAG];
+#define AR_MAX_NAME_LEN (sizeof(arh.ar_name)-1)
+ FILE * arch; /* Stream to archive */
+ size_t size; /* Size of archive member */
+ char magic[SARMAG];
ArchListNode *ln;
- Arch *ar; /* Archive descriptor */
- struct ar_hdr arh; /* archive-member header for reading archive */
- char memName[MAXPATHLEN + 1];
- /* Current member name while hashing. */
+ Arch *ar; /* Archive descriptor */
+ struct ar_hdr arh; /* archive-member header for reading archive */
+ char memName[MAXPATHLEN+1];
+ /* Current member name while hashing. */
/*
* Because of space constraints and similar things, files are archived
@@ -476,7 +486,7 @@
{
/* Try truncated name */
- char copy[AR_MAX_NAME_LEN + 1];
+ char copy[AR_MAX_NAME_LEN+1];
size_t len = strlen(member);
if (len > AR_MAX_NAME_LEN) {
@@ -496,11 +506,11 @@
* no need to allocate extra room for the header we're returning,
* so just declare it static.
*/
- static struct ar_hdr sarh;
+ static struct ar_hdr sarh;
- arch = ArchFindMember(archive, member, &sarh, "r");
+ arch = ArchFindMember(archive, member, &sarh, "r");
- if (arch == NULL) {
+ if (arch == NULL) {
return NULL;
} else {
fclose(arch);
@@ -523,8 +533,8 @@
*/
if ((fread(magic, SARMAG, 1, arch) != 1) ||
(strncmp(magic, ARMAG, SARMAG) != 0)) {
- fclose(arch);
- return NULL;
+ fclose(arch);
+ return NULL;
}
ar = bmake_malloc(sizeof(Arch));
@@ -535,7 +545,7 @@
memName[AR_MAX_NAME_LEN] = '\0';
while (fread((char *)&arh, sizeof(struct ar_hdr), 1, arch) == 1) {
- if (strncmp(arh.ar_fmag, ARFMAG, sizeof(arh.ar_fmag)) != 0) {
+ if (strncmp( arh.ar_fmag, ARFMAG, sizeof(arh.ar_fmag)) != 0) {
/*
* The header is bogus, so the archive is bad
* and there's no way we can recover...
@@ -550,7 +560,7 @@
* boundary, so we need to extract the size of the file from the
* 'size' field of the header and round it up during the seek.
*/
- arh.ar_size[sizeof(arh.ar_size) - 1] = '\0';
+ arh.ar_size[sizeof(arh.ar_size)-1] = '\0';
size = (size_t)strtol(arh.ar_size, NULL, 10);
memcpy(memName, arh.ar_name, sizeof(arh.ar_name));
@@ -569,14 +579,15 @@
* svr4 magic mode; handle it
*/
switch (ArchSVR4Entry(ar, memName, size, arch)) {
- case -1: /* Invalid data */
+ case -1: /* Invalid data */
goto badarch;
- case 0: /* List of files entry */
+ case 0: /* List of files entry */
continue;
- default: /* Got the entry */
+ default: /* Got the entry */
break;
}
- } else {
+ }
+ else {
if (nameend[0] == '/')
nameend[0] = '\0';
}
@@ -590,24 +601,23 @@
if (strncmp(memName, AR_EFMT1, sizeof(AR_EFMT1) - 1) == 0 &&
ch_isdigit(memName[sizeof(AR_EFMT1) - 1])) {
- int elen = atoi(&memName[sizeof(AR_EFMT1) - 1]);
+ int elen = atoi(&memName[sizeof(AR_EFMT1)-1]);
if ((unsigned int)elen > MAXPATHLEN)
- goto badarch;
+ goto badarch;
if (fread(memName, (size_t)elen, 1, arch) != 1)
- goto badarch;
+ goto badarch;
memName[elen] = '\0';
if (fseek(arch, -elen, SEEK_CUR) != 0)
- goto badarch;
+ goto badarch;
if (DEBUG(ARCH) || DEBUG(MAKE)) {
- debug_printf("ArchStat: Extended format entry for %s\n",
- memName);
+ debug_printf("ArchStat: Extended format entry for %s\n", memName);
}
}
#endif
{
- Hash_Entry *he;
+ Hash_Entry *he;
he = Hash_CreateEntry(&ar->members, memName, NULL);
Hash_SetValue(he, bmake_malloc(sizeof(struct ar_hdr)));
memcpy(Hash_GetValue(he), &arh, sizeof(struct ar_hdr));
@@ -696,12 +706,12 @@
entry = (size_t)strtol(&name[1], &eptr, 0);
if ((*eptr != ' ' && *eptr != '\0') || eptr == &name[1]) {
- DEBUG1(ARCH, "Could not parse SVR4 name %s\n", name);
+ DEBUG1(ARCH, "Could not parse SVR4 name %s\n", name);
return 2;
}
if (entry >= ar->fnamesize) {
DEBUG2(ARCH, "SVR4 entry offset %s is greater than %lu\n",
- name, (unsigned long)ar->fnamesize);
+ name, (unsigned long)ar->fnamesize);
return 2;
}
@@ -736,13 +746,13 @@
*/
static FILE *
ArchFindMember(const char *archive, const char *member, struct ar_hdr *arhPtr,
- const char *mode)
+ const char *mode)
{
- FILE *arch; /* Stream to archive */
- int size; /* Size of archive member */
- char magic[SARMAG];
- size_t len, tlen;
- const char *base;
+ FILE * arch; /* Stream to archive */
+ int size; /* Size of archive member */
+ char magic[SARMAG];
+ size_t len, tlen;
+ const char * base;
arch = fopen(archive, mode);
if (arch == NULL) {
@@ -755,8 +765,8 @@
*/
if ((fread(magic, SARMAG, 1, arch) != 1) ||
(strncmp(magic, ARMAG, SARMAG) != 0)) {
- fclose(arch);
- return NULL;
+ fclose(arch);
Home |
Main Index |
Thread Index |
Old Index