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

[Dan Sebald]

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

I did have a vague memory this was discussed within the past couple years.  I
was simply trying to gauge how big of an effort it would be to change to
signed index; again, if it makes things easier somewhere else to have
unsigned, then maybe it is worth it.  JWE would have the best idea of this
given the distribution of posts in that thread.

The cases where negative octave_idx_type has meaning don't concern me too
much.  Those could be weeded out pretty well with the BISTs.  The main issue
as I watched the code compile with unsigned octave_idx_type seems to be its
use in a header file that is included everywhere.  I think it is a header file
that does a relational comparison on reference counting.  That's probably
where the immediate seg-fault comes from...

I wondered if it would be a good idea to make this a macro or inline, so then
I just tried putting __builtin_smull_overflow() for fun, and it worked. 
Ha-ha, what we need is already present.

So, what I've done is written this change in the form of an inline function
that will utilize the GNULIB hardware-supported routine if possible.  That's
kind of nice.  But, JWE, perhaps you could place the changeset's top matter
near the header file list in an appropriate header file with the right
preprocessor definitions for inclusion.  I don't follow the make process too
closely.

Doing things this way also made me realize one other small detail that kind of
clears things up.  The __builtin_smull_overflow was more robust than what I
had in the previous version.  Because I was using

labs(long int)

the compiler does the following

labs(-9223372036854775808) = -9223372036854775808

That is, because of the two's complement number system there isn't symmetry
for that most negative number.  The result is that the number is unchanged for
labs().  Hence, I had to make the test more complex and account for that
situation; another consequence of using signed index instead of unsigned
index.

Lastly, there are other occurrences of nr time nc in the oct-stream.cc file. 
Do we want to protect those by OCTAVE_IDX_OVERFLOW()?  I'm thinking this
function could find use in many places, even though they will be caught by
"out of memory" or some other condition.

@Rik, I added some BIST tests.  There are probably more that can be added. 
One of them fails though.  You indicated that ::error() doesn't need a return,
so I don't know why that second error message is appearing.  I think it is
because the get_size() is within a try-catch block.  That's probably a bug of
it's own, so I haven't addressed that.


(file #44671)
    _______________________________________________________

Additional Item Attachment:

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


    _______________________________________________________

Reply to this item at:

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

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