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