[Octave-bug-tracker] [bug #54405] octave_idx_type index integer overflow math check doesn't work correctly

[Dan Sebald]

Follow-up Comment #31, bug #54405 (project octave):

It wouldn't surprise me if std::abs() is set up to use the appropriate abs()
function based on variable type.  I'll use that instead.

What you've done is effectively treat the octave_idx_type as a 63-bit unsigned
number, i.e., if nc > 0 and nr > 0.  I was sort of aiming for a general
overflow test (whether that be signed or unsigned) so that it can be replaced
by one of the GNULIB routines from here:

https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Integer-Overflow-Builtins.html

These are already compiled into the Octave executable, so why not use them? 
They are much nicer routines because they make use of the CPU's overflow flags
if readily available.  That is, they simply carry out the multiplication, then
check the CPU's overflow bit in some ALU register or do some conditional jump
on overflow; one machine cycle.

On the other hand, the division, even if it is built in to the processor,
probably takes many machine cycles.  It all depends on the CPU of course:
CISC, RISC, DSP.

Also, I still think this isn't quite correct.  By ignoring those negative
numbers, it can't catch that troublesome conversion from 53-bit mantissa float
to signed long.  Check out this example (from the canonical version at
changeset http://hg.savannah.gnu.org/hgweb/octave/rev/02fbbde15308


octave:32> x = fread (fid, [9223372036854775807, 9223372036854775807],
'single');
octave:33> x = fread (fid, [9223372036854775808, 9223372036854775808],
'single');
octave:34> x = fread (fid, [9223372036854775809, 9223372036854775809],
'single');
octave:35> x = fread (fid, [92233720368547758090, 92233720368547758090],
'single');
error: fread: dimension too large for Octave's index type
error: value on right hand side of assignment is undefined


Notice how long INT_MAX (9223372036854775807) and greater are not being
caught.  When the size gets much bigger, though, the test starts catching the
too-large index.  There is this troublesome group of numbers I've been trying
to illustrate which come about because of that conversion from float to long
int.  Even though they are positive as entered, they end up internally as
negative numbers when converted.  So, the code that checks


if (nr > 0 && nc > 0 ...


is overlooking that scenario.  The out-of-range numbers really do end up
negative at that point.

This fact is one of the reasons I've questioned if it might be better to
utilize the octave_idx_type as unsigned and have them computed as soon as
possible coming out of the interpreter rather than storing the indices as
doubles until the moment before they are used.

Anyway, I don't think get_size() is the proper place to check NR * NC, but I'm
fine with it.  However, I'm attaching another changeset that incorporates the
main ideas from my previous changeset.  Look them over and see why they are
there using the new BISTs of io.tst.  After running the BISTs, revert all the
changes except the io.tst file, recompile and run the io.tst test again.

The tests will also show the try-catch double-error problem.  Do we want that
to be a separate bug?

(file #44680)
    _______________________________________________________

Additional Item Attachment:

File name: octave-fread_index_overflow_check-djs2018aug01.patch Size:5 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?54405>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/