Re: General bug - No out of bounds checking on 3D coordinates

[Hans-Bernhard B. <br...@ph...>]

On Sun, 4 Jul 2004, Ethan A Merritt wrote:

> I have encountered a nasty bug that has been around since forever.
> It is not obvious to me what is the best way to deal with it.
> 
> The basic problem is that the 3D mapping routine map3d_xy() in util3d.c
> stuffs its output into (unsigned int) terminal coordinates.
> But it does so without ever checking that the results are positive numbers.  

Going negative isn't the only possible error.  Going beyond term->xmax or
term->ymax is just as wrong.  Granted, the terminal drivers don't usually
crash or run into endless loops because of it, but it's wrong nonetheless.

And it's not only map3d_xy that does that.  Traditionally, there has been
next to *no* clipping of positions to visible or allowable ranges anywhere
in gnuplot.  Some terminal drivers do their own clipping, and some
graphics elements did learn to clip properly over time.  But generally
it's still a "you get what you asked for" world down there.

See, e.g. the 'set arrow to far away points causes garbage' bug registered
at SF.  In more technical terms, that bug's title should be "arrows don't
clip".

> - Have map3d_xy() call int_error() if the coordinates go negative?

No.  map3d_xy is far too deep in the basement of gnuplot to output a
meaningful int_error() message. 

> - Have map3d_xy() siliently truncate to zero?

> - Have map3d_xy() truncate, but not so silently?

No to both.  For the same reason: map3d_xy() has no way of knowing how to
properly clip positions.  E.g. if the actual thing being drawn is a line
segment, clipping the out-of-range endpoint to the nearest point on the
viewport border will rotate the entire edge.  I'm quite sure that's not
sensible behaviour.

Anyway, IMHO if map3d_xy() were to clip, it should clip *before* it
transforms, i.e. it should clip points to the boundary of its input
coordinate system (the 3D graph box).  But that would kill time-honoured
hacks like 'set view ,,1.2', which deliberately produce points outside
that volume.

gnuplot is consistently inconsistent in the way it uses signed vs.  
unsigned data types.  E.g. in the context at hand, map3d_xy uses
unsigned int for its result:

    *xt = (unsigned int) ((res[0] * xscaler / w) + xmiddle);
    *yt = (unsigned int) ((res[1] * yscaler / w) + ymiddle);

whereas the other major conversion method from normalized, or "view" 3D
coordinate space (x and y those of the page, z orthogonal to it, origin in
the center of it), to terminal coordinates is the TERMCOORD macro:

/* Maps from normalized space to terminal coordinates */
#define TERMCOORD(v,xvar,yvar)                  \
{                                               \
    xvar = ((int)((v)->x * xscaler)) + xmiddle; \
    yvar = ((int)((v)->y * yscaler)) + ymiddle; \
}

-Wsign-compare is by far the most frequently issued warning you get
building gnuplot with a picky compiler setting:

      1  might be used uninitialized in this function
      1 comparison of unsigned expression < 0 is always false
      2 argument  might be clobbered by  or
      4 signed and unsigned type in conditional expression
     10 unused parameter
     21 cast discards qualifiers from pointer target type
     54 (near initialization for )
     54 missing initializer
    111 comparison between signed and unsigned

(The 54 "missing initializer" ones are caused by shortened terminal
structs, i.e. they're completely harmless).

> The cleanest is probably to return an error from map3d_xy, 

Agreed.

> but since there are roughly 100 call sites this would be a *lot* of
> extra code to handle error checking.

Not really.  map3d_xy() is called 41 times:

gnuplot/src $ cscope -L3 map3d_xy | wc -l
     41

The companion function map3d_xyz() doesn't convert all the way to terminal
coordinates, so it doesn't have this particular problem.

IMHO clipping has to be the duty of the callers of map3d_xy, because only
they can possibly know what the right reaction should be, or at least
output an intelligible errors message if they can't seem to find a
reasonable reaction.

-- 
Hans-Bernhard Broeker (br...@ph...)
Even if all the snow were burnt, ashes would remain.