On Friday 04 July 2008 22:36, Jason Curl wrote:
> Denys Vlasenko wrote:
> > On Friday 04 July 2008 09:55, Jason Curl wrote:
> >
> >> Hello,
> >>
> >> I'd like to welcome comments on my first bb patch
> >>
> >
> > It's using spaces for indentation. Otherwise looks ok.
> >
> OK, I'll fix this.
> >
> >> (it's the first I've needed to do since I've started
> >> using 1.0.0). I like the new "man" applet, but it
> >> needs nroff and gtbl. Unfortunately, I didn't prepare
> >> the c++ compiler and the gnu tools won't compile.
> >> No worry though, I've updated busybox to support
> >> "cat" pages also. This requires less space than
> >> installing all the GNU tools.
> >>
> >
> > Ideally, some simplified replacement for nroff+gtbl
> > would be cool.
> >
>
> I've seem to found a bug while testing man in a bit more detail. It
> looks like the line
> alloc_mp = 10;
> /* this doesn't appear to initialise the array to NULL pointers */
> man_path_list = xmalloc(10 * sizeof(man_path_list[0]));
> count_mp = 0;
> man_path_list[0] = xstrdup(getenv("MANPATH"));
>
> doesn't initialise the array to zero in my case (indicating NULL
> pointers). So as this array gets built up while parsing "/etc/man.conf"
> it assumes that it was previously NULL.
> if (strcmp("MANPATH", line) == 0) {
> man_path_list[count_mp] = xstrdup(value);
> count_mp++;
> if (alloc_mp == count_mp) {
> alloc_mp += 10;
> man_path_list = xrealloc(man_path_list, alloc_mp
> * sizeof(man_path_list[0]));
> }
> /* thus man_path_list is always NULL terminated */
>
> That last comment doesn't seem to hold true.
>
> Then the final while loop checks
> while ((cur_path = man_path_list[cur_mp++]) != NULL) {
>
> and it turns out that uninitialised data is compared against NULL which
> fails, enters the loop again and dies with a segfault.
>
> So the solution appears to be
> if (cf) {
> ..
> }
> man_path_list[count_mp] = NULL;
>
> This fixes my segfault.
Plase try this patch. It has an advantage of also making code smaller.
--
vda
diff -d -urpN busybox.5/miscutils/man.c busybox.6/miscutils/man.c
--- busybox.5/miscutils/man.c 2008-06-28 19:39:21.000000000 +0200
+++ busybox.6/miscutils/man.c 2008-07-04 23:14:51.000000000 +0200
@@ -75,7 +75,7 @@ int man_main(int argc ATTRIBUTE_UNUSED,
char *sec_list;
char *cur_path, *cur_sect;
char *line, *value;
- int count_mp, alloc_mp, cur_mp;
+ int count_mp, cur_mp;
int opt, not_found;
opt_complementary = "-1"; /* at least one argument */
@@ -83,8 +83,8 @@ int man_main(int argc ATTRIBUTE_UNUSED,
argv += optind;
sec_list = xstrdup("1:2:3:4:5:6:7:8:9");
- alloc_mp = 10;
- man_path_list = xmalloc(10 * sizeof(man_path_list[0]));
+ /* Last valid man_path_list[] is [0x10] */
+ man_path_list = xzalloc(0x11 * sizeof(man_path_list[0]));
count_mp = 0;
man_path_list[0] = xstrdup(getenv("MANPATH"));
if (man_path_list[0])
@@ -109,11 +109,13 @@ int man_main(int argc ATTRIBUTE_UNUSED,
if (strcmp("MANPATH", line) == 0) {
man_path_list[count_mp] = xstrdup(value);
count_mp++;
- if (alloc_mp == count_mp) {
- alloc_mp += 10;
- man_path_list = xrealloc(man_path_list, alloc_mp * sizeof(man_path_list[0]));
+ /* man_path_list is NULL terminated */
+ man_path_list[count_mp] = NULL;
+ if (!(count_mp & 0xf)) { /* 0x10, 0x20 etc */
+ /* so that last valid man_path_list[] is [count_mp + 0x10] */
+ man_path_list = xrealloc(man_path_list,
+ (count_mp + 0x11) * sizeof(man_path_list[0]));
}
- /* thus man_path_list is always NULL terminated */
}
if (strcmp("MANSECT", line) == 0) {
free(sec_list);
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox
