Re: [Fwd: cross-thread violation on rb_gc()]

Hi,

At Wed, 18 Oct 2006 10:52:11 +0900,
Nobuyoshi Nakada wrote in [ruby-core:09228]:
> 1) rewrite those functions not to use ruby's GC,

Well, have I posted this patch?

The usage of ALLOC() in glob_helper(), calling without binding the
result to any VALUE, can cause memory leak.


Index: dir.c
===================================================================
RCS file: /pub/cvs/ruby/dir.c,v
retrieving revision 1.159
diff -U 2 -p -r1.159 dir.c
--- dir.c	25 Sep 2006 14:19:57 -0000	1.159
+++ dir.c	18 Oct 2006 04:03:27 -0000
@@ -923,5 +923,9 @@ sys_warning_1(const char* mesg)
 #define GLOB_VERBOSE	(1UL << (sizeof(int) * CHAR_BIT - 1))
 #define sys_warning(val) \
-    ((flags & GLOB_VERBOSE) && rb_protect((VALUE
(*)_((VALUE)))sys_warning_1, (VALUE)(val), 0))
+    (void)((flags & GLOB_VERBOSE) && rb_protect((VALUE
(*)_((VALUE)))sys_warning_1, (VALUE)(val), 0))
+
+#define GLOB_ALLOC(type) (type *)malloc(sizeof(type))
+#define GLOB_ALLOC_N(type, n) (type *)malloc(sizeof(type) * (n))
+#define GLOB_JUMP_TAG(status) ((status == -1) ? rb_memerror() :
rb_jump_tag(status))

 /* System call with warning */
@@ -1059,4 +1063,6 @@ struct glob_pattern {
 };

+static void glob_free_pattern(struct glob_pattern *list);
+
 static struct glob_pattern *
 glob_make_pattern(const char *p, int flags)
@@ -1066,5 +1072,6 @@ glob_make_pattern(const char *p, int fla

     while (*p) {
-	tmp = ALLOC(struct glob_pattern);
+	tmp = GLOB_ALLOC(struct glob_pattern);
+	if (!tmp) goto error;
 	if (p[0] == '*' && p[1] == '*' && p[2] == '/') {
 	    /* fold continuous RECURSIVEs (needed in glob_helper) */
@@ -1076,5 +1083,9 @@ glob_make_pattern(const char *p, int fla
 	else {
 	    const char *m = find_dirsep(p, flags);
-	    char *buf = ALLOC_N(char, m-p+1);
+	    char *buf = GLOB_ALLOC_N(char, m-p+1);
+	    if (!buf) {
+		free(tmp);
+		goto error;
+	    }
 	    memcpy(buf, p, m-p);
 	    buf[m-p] = '\0';
@@ -1094,5 +1105,11 @@ glob_make_pattern(const char *p, int fla
     }

-    tmp = ALLOC(struct glob_pattern);
+    tmp = GLOB_ALLOC(struct glob_pattern);
+    if (!tmp) {
+      error:
+	*tail = 0;
+	glob_free_pattern(list);
+	return 0;
+    }
     tmp->type = dirsep ? MATCH_DIR : MATCH_ALL;
     tmp->str = 0;
@@ -1119,6 +1136,7 @@ join_path(const char *path, int dirsep,
 {
     long len = strlen(path);
-    char *buf = ALLOC_N(char, len+strlen(name)+(dirsep?1:0)+1);
+    char *buf = GLOB_ALLOC_N(char, len+strlen(name)+(dirsep?1:0)+1);

+    if (!buf) return 0;
     memcpy(buf, path, len);
     if (dirsep) {
@@ -1230,4 +1248,5 @@ glob_helper(
 	if (match_dir && isdir == YES) {
 	    char *tmp = join_path(path, dirsep, "");
+	    if (!tmp) return -1;
 	    status = glob_call_func(func, tmp, arg);
 	    free(tmp);
@@ -1245,6 +1264,10 @@ glob_helper(
 	for (dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) {
 	    char *buf = join_path(path, dirsep, dp->d_name);
-
 	    enum answer new_isdir = UNKNOWN;
+
+	    if (!buf) {
+		status = -1;
+		break;
+	    }
 	    if (recursive && strcmp(dp->d_name, ".") != 0 &&
strcmp(dp->d_name, "..") != 0
 		&& fnmatch("*", dp->d_name, flags) == 0) {
@@ -1259,5 +1282,9 @@ glob_helper(
 	    }

-	    new_beg = new_end = ALLOC_N(struct glob_pattern *, (end - beg) * 2);
+	    new_beg = new_end = GLOB_ALLOC_N(struct glob_pattern *, (end - beg) * 2);
+	    if (!new_beg) {
+		status = -1;
+		break;
+	    }

 	    for (cur = beg; cur < end; ++cur) {
@@ -1285,5 +1312,6 @@ glob_helper(
 	struct glob_pattern **copy_beg, **copy_end, **cur2;

-	copy_beg = copy_end = ALLOC_N(struct glob_pattern *, end - beg);
+	copy_beg = copy_end = GLOB_ALLOC_N(struct glob_pattern *, end - beg);
+	if (!copy_beg) return -1;
 	for (cur = beg; cur < end; ++cur)
 	    *copy_end++ = (*cur)->type == PLAIN ? *cur : 0;
@@ -1293,9 +1321,18 @@ glob_helper(
 		char *buf;
 		char *name;
-		name = ALLOC_N(char, strlen((*cur)->str) + 1);
+		name = GLOB_ALLOC_N(char, strlen((*cur)->str) + 1);
+		if (!name) {
+		    status = -1;
+		    break;
+		}
 		strcpy(name, (*cur)->str);
 		if (escape) remove_backslashes(name);

-		new_beg = new_end = ALLOC_N(struct glob_pattern *, end - beg);
+		new_beg = new_end = GLOB_ALLOC_N(struct glob_pattern *, end - beg);
+		if (!new_beg) {
+		    free(name);
+		    status = -1;
+		    break;
+		}
 		*new_end++ = (*cur)->next;
 		for (cur2 = cur + 1; cur2 < copy_end; ++cur2) {
@@ -1308,4 +1345,9 @@ glob_helper(
 		buf = join_path(path, dirsep, name);
 		free(name);
+		if (!buf) {
+		    free(new_beg);
+		    status = -1;
+		    break;
+		}
 		status = glob_helper(buf, 1, UNKNOWN, UNKNOWN, new_beg, new_end,
flags, func, arg);
 		free(buf);
@@ -1339,9 +1381,14 @@ ruby_glob0(const char *path, int flags,

     n = root - start;
-    buf = ALLOC_N(char, n + 1);
+    buf = GLOB_ALLOC_N(char, n + 1);
+    if (!buf) return -1;
     MEMCPY(buf, start, char, n);
     buf[n] = '\0';

     list = glob_make_pattern(root, flags);
+    if (!list) {
+	free(buf);
+	return -1;
+    }
     status = glob_helper(buf, 0, UNKNOWN, UNKNOWN, &list, &list + 1,
flags, func, arg);
     glob_free_pattern(list);
@@ -1387,5 +1434,5 @@ rb_glob(const char *path, void (*func)(c
 {
     int status = rb_glob2(path, 0, func, arg);
-    if (status) rb_jump_tag(status);
+    if (status) GLOB_JUMP_TAG(status);
 }

@@ -1420,7 +1467,8 @@ ruby_brace_expand(const char *str, int f

     if (lbrace && rbrace) {
-	char *buf = ALLOC_N(char, strlen(s) + 1);
+	char *buf = GLOB_ALLOC_N(char, strlen(s) + 1);
 	long shift;

+	if (!buf) return -1;
 	memcpy(buf, s, lbrace-s);
 	shift = (lbrace-s);
@@ -1504,5 +1552,5 @@ rb_push_glob(VALUE str, int flags) /* '\
 	int status = push_glob(ary, RSTRING_PTR(str) + offset, flags);
 	char *p, *pend;
-	if (status) rb_jump_tag(status);
+	if (status) GLOB_JUMP_TAG(status);
 	if (offset >= RSTRING_LEN(str)) break;
 	p = RSTRING_PTR(str) + offset;
@@ -1528,5 +1576,5 @@ dir_globs(long argc, VALUE *argv, int fl
 	StringValue(str);
 	status = push_glob(ary, RSTRING_PTR(str), flags);
-	if (status) rb_jump_tag(status);
+	if (status) GLOB_JUMP_TAG(status);
     }

Index: win32/win32.c
===================================================================
RCS file: /pub/cvs/ruby/win32/win32.c,v
retrieving revision 1.196
diff -U 2 -p -r1.196 win32.c
--- win32/win32.c	18 Oct 2006 14:02:59 -0000	1.196
+++ win32/win32.c	19 Oct 2006 00:37:02 -0000
@@ -411,5 +411,6 @@ init_env(void)
 	return;
     }
-    NTLoginName = ALLOC_N(char, len+1);
+    NTLoginName = (char *)malloc(len+1);
+    if (!NTLoginName) return;
     strncpy(NTLoginName, env, len);
     NTLoginName[len] = '\0';
@@ -1060,8 +1061,10 @@ insert(const char *path, VALUE vinfo)
     NtCmdLineElement ***tail = (NtCmdLineElement ***)vinfo;

-    tmpcurr = ALLOC(NtCmdLineElement);
+    tmpcurr = (NtCmdLineElement *)malloc(sizeof(NtCmdLineElement));
+    if (!tmpcurr) return -1;
     MEMZERO(tmpcurr, NtCmdLineElement, 1);
     tmpcurr->len = strlen(path);
-    tmpcurr->str = ALLOC_N(char, tmpcurr->len + 1);
+    tmpcurr->str = (char *)malloc(tmpcurr->len + 1);
+    if (!tmpcurr->str) return -1;
     tmpcurr->flags |= NTMALLOC;
     strcpy(tmpcurr->str, path);
@@ -1085,7 +1088,8 @@ cmdglob(NtCmdLineElement *patt, NtCmdLin
     char *p;
     NtCmdLineElement **last = tail;
+    int status;

     if (patt->len >= MAXPATHLEN)
-	buf = ruby_xmalloc(patt->len + 1);
+	if (!(buf = malloc(patt->len + 1))) return 0;

     strncpy (buf, patt->str, patt->len);
@@ -1094,9 +1098,9 @@ cmdglob(NtCmdLineElement *patt, NtCmdLin
 	if (*p == '\\')
 	    *p = '/';
-    ruby_brace_glob(buf, 0, insert, (VALUE)&tail);
+    status = ruby_brace_glob(buf, 0, insert, (VALUE)&tail);
     if (buf != buffer)
 	free(buf);

-    if (last == tail) return 0;
+    if (status || last == tail) return 0;
     if (patt->flags & NTMALLOC)
 	free(patt->str);
@@ -1322,6 +1326,6 @@ rb_w32_cmdvector(const char *cmd, char *
 	}

-	curr = ALLOC(NtCmdLineElement);
-	MEMZERO(curr, NtCmdLineElement, 1);
+	curr = (NtCmdLineElement *)calloc(sizeof(NtCmdLineElement), 1);
+	if (!curr) goto do_nothing;
 	curr->str = base;
 	curr->len = len;
@@ -1348,5 +1352,16 @@ rb_w32_cmdvector(const char *cmd, char *

     len = (elements+1)*sizeof(char *) + strsz;
-    buffer = ALLOC_N(char, len);
+    buffer = (char *)malloc(len);
+    if (!buffer) {
+      do_nothing:
+	while (curr = cmdhead) {
+	    cmdhead = curr->next;
+	    if (curr->flags & NTMALLOC) free(curr->str);
+	    free(curr);
+	}
+	free(cmdline);
+	for (vptr = *vec; *vptr; ++vptr);
+	return vptr - *vec;
+    }

     //
@@ -1448,4 +1463,5 @@ rb_w32_opendir(const char *filename)
     if (fh == INVALID_HANDLE_VALUE) {
 	errno = map_errno(GetLastError());
+	free(p);
 	return NULL;
     }
@@ -1457,7 +1473,12 @@ rb_w32_opendir(const char *filename)

     idx = strlen(fd.cFileName)+1;
-    p->start = ALLOC_N(char, idx);
+    if (!(p->start = (char *)malloc(idx)) || !(p->bits = (char *)malloc(1))) {
+      error:
+	rb_w32_closedir(p);
+	FindClose(fh);
+	errno = ENOMEM;
+	return NULL;
+    }
     strcpy(p->start, fd.cFileName);
-    p->bits = ALLOC_N(char, 1);
     p->bits[0] = 0;
     if (fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
@@ -1485,5 +1506,5 @@ rb_w32_opendir(const char *filename)
 	Renew (p->start, idx+len+1, char);
 	if (p->start == NULL) {
-            rb_fatal ("opendir: malloc failed!\n");
+	    goto error;
 	}
 	strcpy(&p->start[idx], fd.cFileName);
@@ -1492,5 +1513,5 @@ rb_w32_opendir(const char *filename)
 	    Renew (p->bits, p->nfiles / 4 + 1, char);
 	    if (p->bits == NULL) {
-		rb_fatal ("opendir: malloc failed!\n");
+		goto error;
 	    }
 	    p->bits[p->nfiles / 4] = 0;
@@ -3982,8 +4003,10 @@ rb_w32_get_environ(void)
 	if (*env != '=') num++;

-    myenvtop = ALLOC_N(char*, num + 1);
+    myenvtop = (char **)malloc(sizeof(char *) * (num + 1));
     for (env = envtop, myenv = myenvtop; *env; env += strlen(env) + 1) {
 	if (*env != '=') {
-	    *myenv = ALLOC_N(char, strlen(env) + 1);
+	    if (!(*myenv = (char *)malloc(strlen(env) + 1))) {
+		break;
+	    }
 	    strcpy(*myenv, env);
 	    myenv++;


--
Nobu Nakada