Hi,

At Mon, 9 Sep 2002 18:07:07 +0900,
Michal Rokos wrote:
> 	magic is allocated as well, but sometimes it's not freed.

Unfortunately, it's not enough.

glob_helper() calls arbitrary functions, even ruby blocks
passed to Dir.glob, so this function must be exception safe.
Also, closedir() must be ensured.

I thought 2 approaches, 1) wrap resources by VALUE, 2) use
rb_protect() all *func and recursive call, but feel the latter
is too complex.


Index: dir.c =================================================================== RCS file: /cvs/ruby/src/ruby/dir.c,v retrieving revision 1.71 diff -u -2 -p -r1.71 dir.c --- dir.c 15 Jun 2002 10:24:38 -0000 1.71 +++ dir.c 10 Sep 2002 11:30:03 -0000 @@ -598,14 +598,11 @@ has_magic(s, send, flags) } -static char* +static VALUE extract_path(p, pend) - char *p, *pend; + const char *p, *pend; { - char *alloc; int len; len = pend - p; - alloc = ALLOC_N(char, len+1); - memcpy(alloc, p, len); if (len > 1 && pend[-1] == '/' #if defined DOSISH @@ -613,18 +610,15 @@ extract_path(p, pend) #endif ) { - alloc[len-1] = 0; - } - else { - alloc[len] = 0; + --len; } - return alloc; + return rb_str_new(p, len); } -static char* +static VALUE extract_elem(path) - char *path; + const char *path; { - char *pend; + const char *pend; pend = strchr(path, '/'); @@ -654,4 +648,25 @@ remove_backslashes(p) #endif +struct d_link { + char *path; + struct d_link *next; +}; + +static void d_link_free _((void *)); +static void +d_link_free(p) + void *p; +{ + struct d_link *tmp, *link = p; + while (link) { + tmp = link; + link = link->next; + free(tmp->path); + free(tmp); + } +} + +#define DISPOSE(s) (free(RSTRING(s)->ptr), rb_gc_force_recycle(s)) + static void glob_helper(path, sub, flags, func, arg) @@ -687,50 +702,54 @@ glob_helper(path, sub, flags, func, arg) m = strchr(p, '/'); if (has_magic(p, m, flags)) { - char *dir, *base, *magic, *buf; + const char *dir; DIR *dirp; struct dirent *dp; int recursive = 0; - - struct d_link { - char *path; - struct d_link *next; - } *tmp, *link = 0; + VALUE dirv, base, magic, buf, link; + struct d_link *tmp, **tail; base = extract_path(path, p); if (path == p) dir = "."; - else dir = base; + else dir = RSTRING(base)->ptr; magic = extract_elem(p); if (stat(dir, &st) < 0) { if (errno != ENOENT) rb_sys_warning(dir); - free(base); + DISPOSE(base); break; } if (S_ISDIR(st.st_mode)) { - if (m && strcmp(magic, "**") == 0) { - int n = strlen(base); + if (m && strcmp(RSTRING(magic)->ptr, "**") == 0) { + int n = RSTRING(base)->len; recursive = 1; - buf = ALLOC_N(char, n+strlen(m)+3); - sprintf(buf, "%s%s", base, *base ? m : m+1); - glob_helper(buf, buf+n, flags, func, arg); - free(buf); + buf = rb_str_new(0, n+strlen(m)+2); + sprintf(RSTRING(buf)->ptr, "%s%s", RSTRING(base)->ptr, RSTRING(base)->len ? m : m+1); + glob_helper(RSTRING(buf)->ptr, RSTRING(buf)->ptr+n, flags, func, arg); + DISPOSE(buf); } dirp = opendir(dir); if (dirp == NULL) { rb_sys_warning(dir); - free(base); + DISPOSE(base); + DISPOSE(magic); break; } + dirv = Data_Wrap_Struct(rb_cData, 0, (RUBY_DATA_FUNC)closedir, dirp); } else { - free(base); + DISPOSE(base); + DISPOSE(magic); break; } - + + link = Data_Wrap_Struct(rb_cData, 0, d_link_free, 0); + tail = (struct d_link **)&DATA_PTR(link); + dir = RSTRING(base)->ptr; #if defined DOSISH -#define BASE (*base && !((isdirsep(*base) && !base[1]) || (base[1] == ':' && isdirsep(base[2]) && !base[3]))) +#define BASE (*dir && !((isdirsep(*dir) && !dir[1]) || (dir[1] == ':' && isdirsep(dir[2]) && !dir[3]))) #else -#define BASE (*base && !(*base == '/' && !base[1])) +#define BASE (*dir && !(*dir == '/' && !dir[1])) #endif + dir = (BASE) ? "/" : ""; for (dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { @@ -738,59 +757,67 @@ glob_helper(path, sub, flags, func, arg) if (strcmp(".", dp->d_name) == 0 || strcmp("..", dp->d_name) == 0) continue; - buf = ALLOC_N(char, strlen(base)+NAMLEN(dp)+strlen(m)+6); - sprintf(buf, "%s%s%s", base, (BASE) ? "/" : "", dp->d_name); - if (lstat(buf, &st) < 0) { - if (errno != ENOENT) rb_sys_warning(buf); + buf = rb_str_new(0, RSTRING(base)->len + NAMLEN(dp) + strlen(m) + 5); + sprintf(RSTRING(buf)->ptr, "%s%s%s", RSTRING(base)->ptr, dir, dp->d_name); + if (lstat(RSTRING(buf)->ptr, &st) < 0) { + if (errno != ENOENT) rb_sys_warning(RSTRING(buf)->ptr); continue; } if (S_ISDIR(st.st_mode)) { - char *t = buf+strlen(buf); + char *t = RSTRING(buf)->ptr + strlen(RSTRING(buf)->ptr); strcpy(t, "/**"); strcpy(t+3, m); - glob_helper(buf, t, flags, func, arg); + glob_helper(RSTRING(buf)->ptr, t, flags, func, arg); } - free(buf); + DISPOSE(buf); continue; } - if (fnmatch(magic, dp->d_name, flags) == 0) { - buf = ALLOC_N(char, strlen(base)+NAMLEN(dp)+2); - sprintf(buf, "%s%s%s", base, (BASE) ? "/" : "", dp->d_name); + if (fnmatch(RSTRING(magic)->ptr, dp->d_name, flags) == 0) { + buf = rb_str_new(0, RSTRING(base)->len + NAMLEN(dp) + 1); + sprintf(RSTRING(buf)->ptr, "%s%s%s", RSTRING(base)->ptr, dir, dp->d_name); if (!m) { - (*func)(buf, arg); - free(buf); + (*func)(RSTRING(buf)->ptr, arg); + DISPOSE(buf); continue; } tmp = ALLOC(struct d_link); - tmp->path = buf; - tmp->next = link; - link = tmp; + tmp->path = RSTRING(buf)->ptr; + tmp->next = 0; + *tail = tmp; + tail = &tmp->next; + RSTRING(buf)->ptr = 0; + RSTRING(buf)->len = 0; + rb_gc_force_recycle(buf); } } closedir(dirp); - free(base); - free(magic); - if (link) { - while (link) { - if (stat(link->path, &st) == 0) { + DATA_PTR(dirv) = 0; + rb_gc_force_recycle(dirv); + DISPOSE(base); + DISPOSE(magic); + if ((tmp = DATA_PTR(link)) != 0) { + do { + if (stat(tmp->path, &st) == 0) { if (S_ISDIR(st.st_mode)) { - int len = strlen(link->path); + int len = strlen(tmp->path); int mlen = strlen(m); - char *t = ALLOC_N(char, len+mlen+1); + VALUE t = rb_str_new(0, len+mlen); - sprintf(t, "%s%s", link->path, m); - glob_helper(t, t+len, flags, func, arg); - free(t); + sprintf(RSTRING(t)->ptr, "%s%s", tmp->path, m); + glob_helper(RSTRING(t)->ptr, RSTRING(t)->ptr+len, flags, func, arg); + DISPOSE(t); } } else { - rb_sys_warning(link->path); + rb_sys_warning(tmp->path); } - tmp = link; - link = link->next; + DATA_PTR(link) = tmp->next; free(tmp->path); free(tmp); - } + } while ((tmp = DATA_PTR(link)) != 0); + rb_gc_force_recycle(link); break; } + d_link_free(DATA_PTR(link)); + rb_gc_force_recycle(link); } p = m; @@ -856,5 +883,5 @@ push_braces(ary, s, flags) int flags; { - char *buf; + VALUE buf; char *p, *t, *b; char *lbrace, *rbrace; @@ -880,8 +907,7 @@ push_braces(ary, s, flags) if (lbrace) { - int len = strlen(s); - buf = xmalloc(len + 1); - memcpy(buf, s, lbrace-s); - b = buf + (lbrace-s); + buf = rb_str_new(0, strlen(s)); + memcpy(RSTRING(buf)->ptr, s, lbrace-s); + b = RSTRING(buf)->ptr + (lbrace-s); p = lbrace; while (*p != '}') { @@ -893,7 +919,8 @@ push_braces(ary, s, flags) memcpy(b, t, p-t); strcpy(b+(p-t), rbrace+1); - push_braces(ary, buf, flags); + push_braces(ary, RSTRING(buf)->ptr, flags); } - free(buf); + free(RSTRING(buf)->ptr); + rb_gc_force_recycle(buf); } else { @@ -910,9 +937,9 @@ rb_push_glob(str, flags) { char *p, *pend; - char *buf; char *t; int nest, maxnest; int noescape = flags & FNM_NOESCAPE; VALUE ary; + VALUE buf; if (rb_block_given_p()) @@ -922,5 +949,5 @@ rb_push_glob(str, flags) SafeStringValue(str); - buf = xmalloc(RSTRING(str)->len + 1); + buf = rb_str_new(0, RSTRING(str)->len); p = RSTRING(str)->ptr; @@ -928,5 +955,5 @@ rb_push_glob(str, flags) while (p < pend) { - t = buf; + t = RSTRING(buf)->ptr; nest = maxnest = 0; while (p < pend && isdelim(*p)) p++; @@ -942,12 +969,13 @@ rb_push_glob(str, flags) *t = '\0'; if (maxnest == 0) { - push_globs(ary, buf, flags); + push_globs(ary, RSTRING(buf)->ptr, flags); } else if (nest == 0) { - push_braces(ary, buf, flags); + push_braces(ary, RSTRING(buf)->ptr, flags); } /* else unmatched braces */ } - free(buf); + free(RSTRING(buf)->ptr); + rb_gc_force_recycle(buf); return ary;
-- Nobu Nakada