I'm doing a code review fixing up some things while I'm going. Code in the intricate core parts will be untouched until discussed here though.
On line 340 of rbtree.c we belive that granparent might be 0, but a few lines down it's dereferenced as if that was not an option. Either the first if in assignment of top is unneeded and should go, or things will blow up.
If PIKE_DEBUG is set it will fatal on grandparenslessness, but that also makes the top assignment above it useless.
(It should be noted that I'm doing this on 7.6 HEAD)
On line 2209 we check if p != NULL and then dereference p on line 2234 without changing p as far as I can see. If we are going to bomb we might as well do it on 2209 and add a debug assert if it's not supposed to happen.
On line 412 of pike_memory.c we check if base is NULL and happily continues regardless until line line 424 where we dereference this shiny NULL.
Perhaps add an assert and remove the if?
Check this:
2440 while (len--) 2441 { 2442 if ((s->r==COLORMAX && 2443 s->g==COLORMAX && 2444 s->b==COLORMAX))
2448 else
Can l be NULL only when s->r == s->g == s->b == COLORMAX? because if not..
2450 d->r=MINIMUM(s->r+l->r,COLORMAX);
... this is looking a bit silly:
2458 if (l) l++;
On line 115 in object.c p->storage_needed can be assigned (char *)NULL. On line 136 and 137 LOW_PARENT_INFO dereferences that if I'm reading it correctly. Should make for nice fireworks.
Shuffler::source_pikestring_make returns the result pointer even if it has been freed. That can't be good?
... if( res->len <= 0 ) { sub_ref(res->str); free(res); } return (struct source *)res; }
In polyfill.c::image_polyfill line 817 polyfill_add is called and if that fails polyfill_free is done. However, polyfill_add also calls polyfill_free internally on the same argument.
At a glance it looks like both could happen on the same pointer in sequence.
On line 4517 of pike_types.c match_types() is run without checking the return value. It does have the side effect of running clear_markers(), but is that really the intention? I checked, and the other 47 times match_types() is called the return value is examined.
In Image/blit.c:
What good are the if sections before else on line 250, 251:
if (x2<0) x2=0; else if (x2>=img->xsize) x2=img->xsize-1; if (y2<0) y2=0; else if (y2>=img->ysize) y2=img->ysize-1;
On line 246 we just checked that that wasn't true:
if( ! (( x2 < 0) || (y2 < 0) || (x1>=img->xsize) || (y1>=img->ysize))) {
builtin_functions.c, around 5629:
} else if (pos && stack[pos]->refs == 1 && stack[pos-1] == stack[pos]->prev) { ... } else { ... if (pos) (dml->prev = stack[pos-1])->refs++; else dml->prev = NULL; ...
pos was just checked against on the level above. That is just extra dead code.
pos AND other stuff, not pos OR other stuff.
Quite. Maybe I shouldn't do this so late in the evening.
pike-devel@lists.lysator.liu.se
-
Martin Nilsson (Opera Mini - AFK!) ＠ Pike (-) developers forum -
Peter Bortas (nu med mindre Opera) ＠ Pike (-) developers forum