bugGNU Octave - Bugs: bug #54414, Not recognizing that indices...

 
 

bug #54414: Not recognizing that indices greater than (roughly) 2^63 or 20 digits are too large

Submitted by:  Dan Sebald <sebald>
Submitted on:  Wed 01 Aug 2018 02:38:47 AM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Patch Submitted Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 01 Aug 2018 10:04:31 PM UTC, comment #7:

"Yes" and "no". The same phenomenon is the core issue in both instances, but these are different pieces of code. Bug #45945 is discussing that LONG_INT -> DOUBLE -> LONG_INT issue in the interpreter. Here in the fread() and get_size() routines, it is the fact that the index is being stored as a double. If Bug #45945 is fixed, it isn't going to fix the fread problem, unless the indices are kept as octave_idx_type the whole way.

This is why I'm suggesting the hunk of code for that region of numbers for which the double loses resolution:

and it is noted as a FIXME for the simple fact the situation may be avoidable. Basically, the above is just a warning that things are on shaky ground with the float to long int conversion.

But, as I look at this, the above doesn't seem to cover all the issues. The above is a loss of resolution, sure, but why the strange wrapping effect when the numbers are supposedly greater than std::numeric_limits<octave_idx_type>::max ()?

So, we know that the following is dodgy:

d, a double, could have been entered with a string, say, one greater than

i.e., [9223372036854775808, 9223372036854775808] and that string gets translated to float and consequently becomes

That's the loss of resolution at work. Eventually as we increase x's string value, the double will test greater than std::numeric_limits<octave_idx_type>::max ().

But why is the following:

returning something zero or negative? It should still be returning some rather large number which will cause the "too big of index" error.

Let's take a look at that nint_big() routine from lo-mappers.cc. There are several of them:

Two things should be considered. 1) Could adding the 0.5 prior to the cast cause an issue? 2) I believe the promotion rules means that the size comparison is done by converting the integer to double, then compare. But perhaps what really should be done is convert the floating point to integer then test. Hold on, let me tweak the nbig_int...

Yes, that looks to have fixed the problem. Take a look at the attached changeset and see if you agree with it. We still need to address the float value being grossly larger than, but then there also needs to be a subtle analysis at that decreased resolution boundary region.

Do we need to cover the

case? The double mantissa is adequate resolution for 32-bit int, but can there be some value, say 1234.56789, that causes problems, i.e., the fraction part takes away integer resolution?

(file #44686)

Dan Sebald <sebald>
Wed 01 Aug 2018 07:23:51 PM UTC, comment #6:

But isn't this just bug #45945 again? In Octave there is no way to enter the number in the interpreter and not have it lose precision with respect to (u)int64 because the interpreter always reads doubles.

The number has already lost one digit of precision. Now find out what eps is.

And if I try and read a number that is distinguishable from the max octave_idx_type then I do get an error.

Rik <rik5>
Project Administrator
Wed 01 Aug 2018 06:06:06 PM UTC, comment #5:

I have the most recent, but it's a 64-bit build.

Dan Sebald <sebald>
Wed 01 Aug 2018 04:54:26 PM UTC, comment #4:

I don't think you're running from the most recent development tip. When I try your code I get

The fix here is only about the zeros() (and friends) functions.

Rik <rik5>
Project Administrator
Wed 01 Aug 2018 04:46:29 PM UTC, comment #3:

That fixes this case, nice:

Yes, the error message could maybe be a little more helpful, but a little thought and it should be evident what's wrong. Here's the sort of hint the gcc compiler displays:

However, this change doesn't fix this example:

The get_size() routine, after the recent change of Bug #54405, is doing its own overflow check. It's repetitive code, with conversions in both get_size() and octave_idx_type_vector_value(). I feel the sooner test in the interpreter is the better place to catch these. However, that could mean noticeable change in the variable holding that index information.

Dan Sebald <sebald>
Wed 01 Aug 2018 04:07:17 PM UTC, comment #2:

For testing, I've been finding it easier to configure with --disable-64 and use 32-bit indices. 2^31 is a lot smaller than 2^63 and makes the issues apparent at more approachable sizes.

Rik <rik5>
Project Administrator
Wed 01 Aug 2018 03:45:35 PM UTC, comment #1:

Tracing this down. The zeros function is in data.cc which calls the fill_matrix function which is also in data.cc. The fill_matrix command calls get_dimensions in libinterp/corefcn/utils.cc. Within get_dimensions there is this line of code

The octave_idx_type_vector_value function is defined for octave_value objects in ov.cc. The prototype for that function is

For converting doubles to octave_idx_type the following code is used.

If the require_int boolean is true then octave checks for overflow during conversion. The default, however, is false which means results aren't checked and the constructor Array<octave_idx_type> is invoked which can map doubles to negative values.

Attached is a small diff which sets the require_int bool to true when called from get_dimensions. It works, but there may be a better option for the fix. In particular, the error message isn't very informative

Running 'make check' passes though with this small change.

A grep through libinterp shows get_dimensions used 40 times, but some of these seem to be for a mex function of the same name which can probably be ignored.

Ignoring the mex functions it is only data.cc, sparse.cc, and ov-cell.cc that use get_dimensions.

Or, maybe this should be tackled in the Array constructor?

(file #44685)

Rik <rik5>
Project Administrator
Wed 01 Aug 2018 02:38:47 AM UTC, original submission:

When a value input to zeros is larger than approximately 2^63, or maybe it is 20 digits, there is no warning about too big and instead the value is treated as 0. E.g.,

Dan Sebald <sebald>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #44685:  54114.diff added by rik5 (582B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by sebald (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-01 sebald Attached File- => Added octave-nint_math_wrap_around-djs2018aug01.patch, #44686
    2018-08-01 rik5 Attached File- => Added 54114.diff, #44685
        StatusNone => Patch Submitted
        Carbon-Copy- => Added jwe

    Back to the top


    Powered by Savane 3.3