bugGNU Octave - Bugs: bug #59094, Possible integer overflow in...

 
 

bug #59094: Possible integer overflow in __ode15__.cc

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Thu 10 Sep 2020 03:40:06 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 06 Oct 2020 06:31:08 PM UTC, comment #4: 

I manually undefined HAVE_SUNSPARSEMATRIX_REALLOCATE to check if the code actually runs (which it does).

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Thu 01 Oct 2020 06:31:21 AM UTC, comment #3: 

I pushed the change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/8b0675c2cfe0

We could add additional overloads for char, unsigned char and signed char as we need them.
The same for the other operations (see comment #2).

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Sat 19 Sep 2020 11:24:30 AM UTC, comment #2: 

The attached patch adds functions that check whether multiplication of two "similar" integers would overflow. These functions wrap around macros provided by gnulib's `intprops` module.

It also adds the necessary check to `__ode15__.cc`.


The rules for integer multiplication between "dis-similar" integers and for function dispatching in C++ might differ (I haven't checked).
But imho it would make code easier to understand (and possibly avoid unintended behavior) if we explicitly casted to a common integer type before such kinds of operation anyway. So I don't think it is necessary to add functions that check integer overflow on operations between "dis-similar" types (like "int type" times "unsigned long int type").


If needed, we could add similar check functions for other potentially critical operations.
Checks are provided by gnulib for:

  • addition
  • subtraction
  • negation
  • multiplication
  • division
  • remainder
  • left shift


See also:
https://www.gnu.org/software/gnulib/manual/html_node/Integer-Type-Overflow.html#Integer-Type-Overflow


Adding jwe to CC because he made the original proposition.


(file #49819)

Markus Mützel <mmuetzel>
Group administrator
Mon 14 Sep 2020 04:36:15 PM UTC, comment #1: 

With the change for bug #55905, the possible integer overflow is mitigated on systems with a version of SUNDIALS that provides "SUNSparseMatrix_Reallocate".

Markus Mützel <mmuetzel>
Group administrator
Thu 10 Sep 2020 03:40:06 PM UTC, original submission:  

It is possible that the multiplication m_num*m_num in the following line in _ode15_.cc can overflow:

        m_sunJacMatrix = SUNSparseMatrix (m_num, m_num, m_num*m_num, CSC_MAT);


https://hg.savannah.gnu.org/hgweb/octave/file/0c9a5eae6c27/libinterp/dldfcn/__ode15__.cc#l404

jwe suggested in bug #58795:

> Gnulib has macros for checking whether integer operations overflow, or we could (conditionally) use things like __builtin_mul_overflow to multiply dimensions and check for overflow.
> I recently noticed that one of the Octave dependencies (maybe it was Sundials?) was using the portable snippets library for this job.  Perhaps we could use that or at least borrow ideas from it.  See, for example,  https://github.com/nemequ/portable-snippets/tree/master/safe-math
> I hope we can refactor so that we limit the number of places where checks like this are needed.


The gnulib macros are probably the ones in "intprops.h":
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/intprops.h;h=df66a3877abbe9e563e2b285297e2702c8106adc;hb=HEAD

IIUC, we try to avoid including gnulib headers directly. Instead we have wrappers for the functions we want to use.
I don't know how to do something similar with these macros.
Is it ok to include that header directly?

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49819:  bug59094_ode15_integer_overflow.patch added by mmuetzel (10KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-06 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-10-01 mmuetzel StatusPatch Submitted Ready For Test
    2020-09-19 mmuetzel Attached File- Added bug59094_ode15_integer_overflow.patch, #49819
        StatusNone Patch Submitted
        Carbon-Copy- Added jwe

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code