bugGNU Octave - Bugs: bug #65712, sort over InF dimension coredumps

 
 

bug #65712: sort over InF dimension coredumps

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Wed 08 May 2024 12:48:03 AM UTC
   
 
Category:  Octave Function 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:  9.2.0 Planned Release:  10.1.0
* Mandatory Fields

Post a Comment

Add a New Comment Rich Markup
   

Discussion

Jump to the original submission

Sun 12 May 2024 02:03:24 AM UTC, comment #51: 

Closing as fixed. To summarize:

  • No crash on any branch, for both one-output and two-output forms.


  • Input validation is different between branches. The bulk of it is OK, but there are some BISTs on default which won't work if run on stable. Hopefully that is not a showstopper.


Arun Giridhar <arungiridhar>
Group Member
Thu 09 May 2024 10:26:18 PM UTC, comment #50: 

The input validation is more challenging to get right than the actual sorting.

I pushed a fix to default here: https://hg.savann ... /rev/a8346c5f6997

Since there is no crash with stable when passed dim = -inf, I didn't make any edit to stable.

There are many functions that are supposed to accept only positive integers for size arguments, but also silently accept 1-i, 2 - pi*i, etc. I put in a check for that.

Arun Giridhar <arungiridhar>
Group Member
Thu 09 May 2024 07:41:06 PM UTC, comment #49: 

we had a lot of code that would error for dim>ndims(x). i don't know if this was old matlab behavior. most functions now permit dim>ndims(x) and just do the appropriate singleton operation over that dim

Nicholas Jankowski <nrjank>
Group Member
Thu 09 May 2024 07:39:41 PM UTC, comment #48: 


>> A = [9 6 3; 8 5 2; 7 4 1];
[C, idx3] = sort (A, 3)

C =

     9     6     3
     8     5     2
     7     4     1


idx3 =

     1     1     1
     1     1     1
     1     1     1

>> [D, idx4] = sort (A, inf)
Error using sort
Dimension argument must be a positive integer.


Nicholas Jankowski <nrjank>
Group Member
Thu 09 May 2024 06:38:43 PM UTC, comment #47: 

Could someone post Matlab output for this code pls?


A = [9 6 3; 8 5 2; 7 4 1];
[C, idx3] = sort (A, 3)
[D, idx4] = sort (A, inf)


Octave currently gives an error for dim > ndims when output index is requested. Not sure yet what that behavior should be.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 08:16:54 PM UTC, comment #46: 

my kingdom for an integrated debugger

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 08:11:33 PM UTC, comment #45: 

This is baffling behavior.

Stable is using nint_value, and this happens when I add cout statements:


octave:2> sort ([1 2; 3 4], inf)
Fsort: dim = 2147483647
ans =
   1   2
   3   4

octave:3> sort ([1 2; 3 4], -inf)
Fsort: dim = 2147483647
ans =
   1   2
   3   4


Apparently nint_value treats both +inf and -inf as intmax(), so they are both treated as +inf.

But on the default branch with its strict_int_value, this happens:

octave:2> sort ([1 2; 3 4], inf)
Fsort: dim = 2147483646
ans =
   1   2
   3   4


octave:3> sort ([1 2; 3 4], -inf)
Fsort: dim = 2147483647
error: sort: DIM must be a positive scalar integer


So how is it that both cases convert -inf to either 2^31-1 or 2^31-2, but one works and the other fails?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 08:05:50 PM UTC, comment #44: 

ok, if it's avoiding crash on both ends, and the output isn't too strange, it's probably sufficient for stable and we can just ensure default gets back to 'correct'.

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 08:02:08 PM UTC, comment #43: 

"-inf" crashes for rc1, does not for for current stable hg tip.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 07:59:24 PM UTC, comment #42: 

at least on the windows 9.1.0 release:

sort(1:4, -Inf)

crashes to desktop just like with +Inf

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 07:55:40 PM UTC, comment #41: 

rik had made a number of input validation improvements to sort in bug #65527, but i think most were pushed to default.  so understandable there's quite a bit of divergence with stable there.

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 07:54:24 PM UTC, comment #40: 


comment #38:

> * Fractional inputs are silently converted to integers, as was expected. This will change with Octave 10 but does not need to be changed now.


They definitely convert in unexpected way for me.
I think code like that would be difficult to debug:


octave:12> d = 2-eps
d = 2.0000
octave:13> sort([1,2;30,4], d)
ans =

    1    2
   30    4

octave:14> sort([1,2;30,4], 1)
ans =

    1    2
   30    4

octave:15> sort([1,2;30,4], 2)
ans =

    1    2
    4   30

octave:16> d
d = 2.0000


But this is beyond the scope of this bug report.

Dmitri.
--





Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 07:51:57 PM UTC, comment #39: 

Oh now I see: there's a difference between stable and the other two for dim = -inf:

On default and bytecode_interpreter:

octave:2> sort ([1 2; 3 4], -inf)
error: sort: DIM must be a positive scalar integer


On stable:

octave:2> sort ([1 2; 3 4], -inf)
ans =
   1   2
   3   4


Can't see why. The checks in Array-base.cc already trap the dim < 0 case, so apparently dim = -inf isn't being caught by that condition? But only on stable, not default.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 07:42:44 PM UTC, comment #38: 

Where are you seeing the -inf crash? I am not able to replicate that on default nor on stable.

I compared the BISTs on default against the ones on stable and tried to run them on stable one by one:

  • Fractional inputs are silently converted to integers, as was expected. This will change with Octave 10 but does not need to be changed now.


  • dim = inf returns the input matrix without a crash.


  • dim being a vector silently takes only the first element, as expected. If the first element is nan, it gives an error. For all positive values of the first element, including inf, it works properly. The pathological failure is with e.g. sort (A, [1 2 nan]) which takes only the 1. But that kind of statement is difficult to enter by accident, only if stress-testing the input validation, so it's probably OK to let it ride until Octave 10?


  • Adding BISTs to data.cc on stable almost always results in a merge conflict and having to merge manually.
Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 07:40:32 PM UTC, comment #37: 


comment #36:

> looks like -inf will still crash.


Works OK for me:


octave:1> sort([1,2;3,4], -inf)
ans =

   1   2
   3   4

octave:2> sort([1,2;3,4], inf)
ans =

   1   2
   3   4

octave:3> sort([1,2;3,4], nan)
error: conversion of NaN to integer value failed


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 07:36:54 PM UTC, comment #36: 

looks like -inf will still crash. -dim is correctly caught with an input validation error.  is there a simple way to catch -inf there without triggering the crash? (something like > abs(ndims()) would maybe redirect too much of the -dim space to return *this instead of error, but something absurd like

dim >= ndims () || dim < -9999

might cover it pending a better fix?

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 07:24:16 PM UTC, comment #35: 

I have just pushed the crash-prevention edit to stable: https://hg.savann ... /rev/f852364698ed

With this, `sort ([1 2; 3 4], inf)` at least does not crash on stable.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 07:17:21 PM UTC, comment #34: 

i realized the bug i was referring to was bug #65527 which was closed and the fix pushed to default. so that also wouldn't address stable.

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 07:15:33 PM UTC, comment #33: 

I was thinking that sort (A, inf) is in the same category as sort (A, n) for n > ndims (A), so it should return A. My initial edit on default got that wrong (was trying to get an error for inf back then) but the BIST for that uncovered the crash.

I can make the cherry-picked changes for the no-crash-on-inf part. I'm testing that on stable and will push in a bit. Those changes are all only in Array-base.cc and easy to merge into default.

But the changes to data.cc cause merge conflicts if I try to make them on stable and then merge to default. Mainly because of missing functions on stable.

I'll do it one at a time and we can test after each.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 06:40:11 PM UTC, comment #32: 

i havent' followed all of the changes, but for compatibility we should try to get:

sort(m, n), where n > ndims(m), should return m

sort(m, n), where n = +/-Inf or NaN, or is nonnumeric, should produce an error.

sort(m, n), where n is nonscalar, whether or not it includes Inf, should produce an error. 

(there's another bug report about functions only looking at dim(1) no matter what size(dim) is. Does an Inf containing vector cause problems?)

it sounds like your comment #31 suggestion for stable would return m instead of error for +Inf. That's better than a crash but catching with an error would be better. But would it catch crashes for -Inf?  (I do see NaN doesn't crash but it gets caught in later processing vs in input validation.)

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 05:10:06 PM UTC, comment #31: 

Not sure how to transplant this to stable, since it uses strict_int_value which doesn't exist on stable.

Maybe doing only one change to Array-base.cc on stable, like "if (dim > ndims())  return *this;" or something similar.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 05:07:24 PM UTC, comment #30: 
Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 04:58:41 PM UTC, comment #29: 

Since it is a crash, the changes should also
be applied to stable, I think.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 04:00:18 PM UTC, comment #28: 

That was probably gcc crosscompiling but even with clang it would not have worked properly. Instead it would have likely given an error about dim needing to be positive.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 02:17:17 PM UTC, comment #27: 

Octave 9.1 crashes on Windows too with sort ([1 2; 3 4], Inf). broadening OS tag.  which compiler were those releases built with?

Nicholas Jankowski <nrjank>
Group Member
Wed 08 May 2024 01:47:45 PM UTC, comment #26: 

This trial change to do the checks before constructing m passes "make check" for me. Will push it later today if it looks OK:


diff -r 47c798415504 liboctave/array/Array-base.cc
--- a/liboctave/array/Array-base.cc     Wed May 08 09:16:05 2024 -0400
+++ b/liboctave/array/Array-base.cc     Wed May 08 09:46:16 2024 -0400
@@ -1806,14 +1806,10 @@ Array<T, Alloc>::sort (int dim, sortmode
   if (dim < 0)
     (*current_liboctave_error_handler) ("sort: invalid dimension");

-  Array<T, Alloc> m (dims ());
-
-  dim_vector dv = m.dims ();
-
-  if (m.numel () < 1)
-    return m;
-
-  if (dim >= dv.ndims ())
+  if (numel () < 1)  // input array is empty,
+    return *this;
+
+  if (dim >= ndims ())
     {
       // The dimension to sort along exceeds the array's dimensions,
       // ==> array is already trivially sorted in such higher dimensions,
@@ -1822,6 +1818,10 @@ Array<T, Alloc>::sort (int dim, sortmode
       return *this;
     }

+  Array<T, Alloc> m (dims ());
+
+  dim_vector dv = m.dims ();
+
   octave_idx_type ns = dv(dim);
   octave_idx_type iter = dv.numel () / ns;
   octave_idx_type stride = 1;


Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:34:48 PM UTC, comment #25: 

Looking at it some more, I have a question. This is the code sequence currently:

  Array<T, Alloc> m (dims ());

  dim_vector dv = m.dims ();

  if (m.numel () < 1)
    return m;

  if (dim >= dv.ndims ())
    return *this


Do we even need to construct m if we are only going to return *this early? I couldn't tell the reason that dim was being compared to dv.ndims() instead of this->ndims().

Is it OK to move that early return before the construction of m?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:22:22 PM UTC, comment #24: 

Pushed the change to return *this: https://hg.savann ... /rev/47c798415504

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:13:52 PM UTC, comment #23: 

The bug in Clang is this:

  • When compiled with -O2 or higher, passing inf to nint_value fails (i.e., inf is treated as nan when they should not be the same).


  • Compiling with -O0 under Clang causes inf and nan to be treated differently as they should.


The above was causing dim = inf inputs to be masked by Clang because nint_value was trapping inf and rejecting it, so when I pushed the BIST last night it broke (segfault) on gcc.

The current version of the code does work properly, as in it allows inf but not nan.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:11:18 PM UTC, comment #22: 

See the permute function in the same file for an example that returns *this in a special case.

John W. Eaton <jwe>
Group administrator
Wed 08 May 2024 01:06:58 PM UTC, comment #21: 

Sorry, crossed with jwe.

I started that fix by returning *this, but then realized I didn't know the semantics of that statement for reference counting, like would it return a reference instead of a value etc. I copied over the code from lower in the function to make a copy. If returning *this is better, I will change that part.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:04:19 PM UTC, comment #20: 

Worked around it by replacing nint_value with strict_int_value, which also traps nan but allows inf even on Clang.

Pushed to https://hg.savann ... /rev/31bc9accbe6e

Ready for test.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:02:34 PM UTC, comment #19: 

I suppose you could use a "testif" block that checks the values of CC and/or CXX from _octave_config_info_, but then you would be adding to the count of "skipped tests based on runtime info".

I'd rather not add to the complexity of the tests to check for compiler and possibly compiler version info.

I don't see what's wrong with just marking the test as a regression if it fails, especially when we are checking something fundamental that should work.

If this is a bug in clang, what exactly is it?

In the patch proposed in comment #16, why do we need to copy elements if the array is already sorted?  Can't we just return *this, which should only increment a reference count?

John W. Eaton <jwe>
Group administrator
Wed 08 May 2024 12:14:12 PM UTC, comment #18: 

Pushed a fix to https://hg.savann ... /rev/8f73f8534d42.

This passes the two new BISTs in gcc, but fails the one with dim = inf in Clang 16. That is probably a bug in Clang based on this thread.

Is there a way to mark a BIST as "known bug for Clang, but should work correctly for GCC otherwise regression"?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 02:52:00 AM UTC, comment #17: 

I think the previous line "dv.resize (dim+1, 1);" in Array-base.cc was the code leading to memory usage. In my patch, I'm returning early now instead.

I'll have to revisit the strict_int_value topic tomorrow. I would certainly prefer sort() to give an error for fractional dimensions, which is what I did except for an extra BIST with inf that started this crash. I think that strict_int check would also solve your comment #13 and comment #14. I do not know what Matlab behavior is -- do they silently convert fractional inputs to the floor?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 02:40:51 AM UTC, comment #16: 

See comment #12 for where the memory was being allocated (inside Array-base.cc to resize the array to fit larger dimensions). I have a patch going through tests now on both compilers.

===

OK testing done with both clang and gcc.

Since this is touching Array, I don't want to push it when I'm starting to feel drowsy lest I break something big. But pls do test it.

Output for me:
On GCC, there's no memory allocation for the 1e9 case. It returns instantly.

octave:1> for i = 1:100, sort (magic(3), inf); end
octave:2> for i = 1:100, sort (magic(3), 1e9); end
octave:3>


On Clang, there's still no memory allocation but it gives its old complaint about inf:

octave:10> for i = 1:100, sort (magic(3), 1e9); end
octave:11> for i = 1:100, sort (magic(3), inf); end
error: sort: DIM must be a positive scalar integer


Patch:

--- a/liboctave/array/Array-base.cc     Tue May 07 20:56:04 2024 -0400
+++ b/liboctave/array/Array-base.cc     Tue May 07 22:32:23 2024 -0400
@@ -1814,7 +1814,19 @@ Array<T, Alloc>::sort (int dim, sortmode
     return m;

   if (dim >= dv.ndims ())
-    dv.resize (dim+1, 1);
+    {
+      // The dimension to sort along exceeds the array's dimensions.
+      // ==> array is already trivially sorted in such higher dimensions.
+      // ==> copy and return.
+
+      T *v = m.rwdata ();
+      const T *ov = data ();
+
+      for (octave_idx_type i = 0; i < m.numel (); i++)
+        v[i] = ov[i];
+
+      return m;
+    }

   octave_idx_type ns = dv(dim);
   octave_idx_type iter = dv.numel () / ns;


Note: This specific patch does not touch data.cc.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 02:23:38 AM UTC, comment #15: 


comment #12:

> @Dmitri, 2^31-1 does not work for me. It gave me OOM. This test:


> for i = 1:100, sort (magic(3), 1e9); end


>
> was occupying up to 8GB for me, fluctuating. So 2^31-1 must be occupying 16 GB. Must be RAM-specific, not because of 32-bit.



I do not understand this. According to matlab's docs
"sort returns A if dim is greater than ndims(A)"
What does it use all the RAM for?

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 02:16:57 AM UTC, comment #14: 

Also this:

octave:48> ones(2, 2-eps)
error: conversion of 2 to int64_t value failed
octave:49> ones(2, 2+eps)
ans =

   1   1
   1   1


(but it starting to be off-topic)

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 02:10:36 AM UTC, comment #13: 

I really think we should require "d" to be positive integer
and Matlab compatibility be damned.
The current situation where d=2-eps is the same as d=1
looks crazy to me.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 02:08:49 AM UTC, comment #12: 

@Dmitri, 2^31-1 does not work for me. It gave me OOM. This test:

for i = 1:100, sort (magic(3), 1e9); end


was occupying up to 8GB for me, fluctuating. So 2^31-1 must be occupying 16 GB. Must be RAM-specific, not because of 32-bit.

Looking in two places: one is the sort DEFUN in data.cc for input validation. The other is "template <typename T, typename Alloc>
Array<T, Alloc> Array<T, Alloc>::sort (int dim, sortmode mode) const" in Array-base.cc, to see why on earth it tries to allocate that memory for nonexistent dimensions.

It has this bit near line 1816, which would explain it:


  if (dim >= dv.ndims ())
    dv.resize (dim+1, 1);


We should probably put in a guard against inf there. I'll test something out next.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:54:59 AM UTC, comment #11: 

We seem to be doing a lot of custom-tailored checks.

  • For ones (inf) it gives: "error: conversion of inf to octave_idx_type value failed"


  • For rand (inf) it gives: "error: out of memory or dimension too large for Octave's index type"


  • For `all (ones(3), inf)` it gives the input matrix back, just as it would for any large dimension more than the input dimensionality.


  • For sort (A, d) where d exceeds the dimension of A but is finite, it works like all() above.


  • For sort (A, inf), it gives an OOM or segfaults.


This could benefit from some streamlining. Is there any one preferred way to do integer conversions and input validation?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:47:52 AM UTC, comment #10: 

Note if I use 2147483647 (which is intmax 2^31-1), it works.

octave:1> sort([1,2;3,4], nan)
error: sort: DIM must be a positive scalar integer
octave:2> sort([1,2;3,4], 2147483647)
ans =

   1   2
   3   4

octave:3> sort([1,2;3,4], inf)
--Type <RET> for more, q to quit, c to continue without paging--c

Thread 9 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffed5555640 (LWP 535012)]
Array<double, std::pmr::polymorphic_allocator<double> >::sort (this=0x7ffebcc52b98, dim=2147483647, mode=ASCENDING) at ../liboctave/array/Array-base.cc:1819
1819          octave_idx_type ns = dv(dim);
(gdb)


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 01:45:57 AM UTC, comment #9: 

No, it causes a segfault for me now with that patch on GCC. Clang is still OK.

Unpatched is still OK, but that means passing dim = inf will crash Octave.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:43:15 AM UTC, comment #8: 

No, it does not:

octave:3> version -hgid
ans = c7b92fa723cc+
octave:4> sort([1,2;3,4], 1)
ans =

   1   2
   3   4

octave:5> sort([1,2;3,4], nan)
error: sort: DIM must be a positive scalar integer
octave:6> sort([1,2;3,4], inf)
--Type <RET> for more, q to quit, c to continue without paging--c

Thread 9 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffed5555640 (LWP 530397)]
Array<double, std::pmr::polymorphic_allocator<double> >::sort (this=0x7ffebceeaa08, dim=2147483647, mode=ASCENDING) at ../liboctave/array/Array-base.cc:1819
1819          octave_idx_type ns = dv(dim);


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 01:33:13 AM UTC, comment #7: 

See if this patch works for you. (I am currently building with gcc and should be able to test in 5-10 minutes.)


diff -r c7b92fa723cc libinterp/corefcn/data.cc
--- a/libinterp/corefcn/data.cc Tue May 07 20:56:04 2024 -0400
+++ b/libinterp/corefcn/data.cc Tue May 07 21:31:55 2024 -0400
@@ -7120,8 +7120,18 @@ ordered lists.
         }
       else
         {
+          // Check dim is scalar, not vector or array.
           if (! args(1).is_scalar_type ())
             error ("sort: DIM must be a positive scalar integer");
+
+          // Check for inputs like sort (magic(3), 1.6).
+          dim = args(1).strict_int_value ("sort: DIM must be a positive scalar integer");
+
+          // Check for inf. Passing dim = inf causes an OOM crash!
+          if (octave::math::isinf (static_cast<double> (dim)))
+            error ("sort: DIM must be finite");
+
+          // Check for nan.
           dim = args(1).nint_value () - 1;
           if (dim < 0)
             error ("sort: DIM must be a positive scalar integer");
@@ -7389,6 +7399,9 @@ ordered lists.
 %!error <Invalid call> sort (1, 2, 3, 4)
 %!error <MODE must be either "ascend" or "descend"> sort (1, "foobar")
 %!error <DIM must be a positive scalar integer> sort (1, [1 2 3])
+%!error <DIM must be a positive scalar integer> sort ([1 2; 3 4], 1.234)
+%!error <DIM must be finite> sort ([1 2; 3 4], inf)
+%!error <DIM must be a positive scalar integer> sort ([1 2; 3 4], nan)
 %!error <DIM argument must precede MODE argument> sort (1, "ascend", 1)
 %!error <MODE must be a string> sort (1, 1, 1)
 %!error <MODE must be either "ascend" or "descend"> sort (1, 1, "foobar")


Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:29:38 AM UTC, comment #6: 

I just compiled with clang (and -O0) and it is crashing:

octave:1> __octave_config_info__ ("CC")
ans = clang
octave:2> __octave_config_info__ ("CXX")
ans = clang++ -std=gnu++17
octave:3> __octave_config_info__ ("CXXFLAGS")
ans = -ggdb3 -O0 -march=native -mtune=native -mavx -mavx2
octave:4> sort([1,2;3,4])
ans =

   1   2
   3   4

octave:5> sort([1,2;3,4], inf)
--Type <RET> for more, q to quit, c to continue without paging--c

Thread 9 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffcd318640 (LWP 467301)]
0x00007ffff3599078 in Array<double, std::pmr::polymorphic_allocator<double> >::sort (this=0x7fffbcd5b7c8, dim=2147483647, mode=ASCENDING) at ../liboctave/array/Array-base.cc:1819
1819          octave_idx_type ns = dv(dim);
(gdb)


clang version 17.0.6 (CentOS 17.0.6-5.el9)

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 01:16:49 AM UTC, comment #5: 

From ov-base.cc:

int
octave_base_value::nint_value (bool frc_str_conv) const
{
  double d = 0.0;

  try
    {
      d = double_value (frc_str_conv);
    }
  catch (octave::execution_exception& ee)
    {
      err_wrong_type_arg (ee, "octave_base_value::nint_value ()", type_name ());
    }

  if (octave::math::isnan (d))
    error ("conversion of NaN to integer value failed");

  return static_cast<int> (octave::math::fix (d));
}


It is checking only for isnan not isinf, so the fact that Clang is lumping inf with nan might be a Clang bug.

Either way, we need better safeguarding against inf inputs causing crashes.

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:14:12 AM UTC, comment #4: 

Fedora buildbots have gcc version 14.0.1 20240411 (Red Hat 14.0.1-0) (GCC) and clang version 18.1.1 (Fedora 18.1.1-1.fc40).

On my workstation I tried with gcc version 11.4.1 20231218 (Red Hat 11.4.1-3) (GCC) and gcc version 13.2.1 20231205 (Red Hat 13.2.1-6) (GCC). All of them crash.

As I mentioned octave on Fedora 40 aarch64 does not crash,
though it takes a long time to return. The same gcc as buildbots.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 01:02:44 AM UTC, comment #3: 

I backed it out while this is being dug into.

So on Clang, this is what I found:

  • If I remove the line with nint_value and rely only on strict_int_value, then inf input causes it to try to occupy all memory. So I left the nint_value line in place as it was.


  • If I remove the first line about is_scalar_value, then it erroneously allows vector inputs for dim, so that line is needed too.


  • I added the strict_int_value check to stop fractional dimensions. Then I added BISTs, verified it all worked on both branches, and pushed. It promptly failed on a different compiler.


Which of the many integer conversion functions guard against inf inputs? Evidently nint_value does that only for some compilers. (?!)

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 01:00:58 AM UTC, comment #2: 

I see that with stable as well:


octave:1> version -hgid
ans = 8e555ddce800
octave:2> sort ([1 2; 3 4], inf)
fatal: caught signal Segmentation fault -- stopping myself...
Segmentation fault (core dumped)


Surprisingly on aarch64 (Fedora 40) it "works"

octave:1> version -hgid
ans = 8e555ddce800
octave:2> sort ([1 2; 3 4], inf)
ans =

   1   2
   3   4

octave:3> version
ans = 9.1.91
octave:4> ver
----------------------------------------------------------------------
GNU Octave Version: 9.1.91 (hg id: 8e555ddce800)
GNU Octave License: GNU General Public License
Operating System: Linux 6.8.8-300.fc40.aarch64 #1 SMP PREEMPT_DYNAMIC Sat Apr 27 18:11:03 UTC 2024 aarch64
----------------------------------------------------------------------
no packages installed.


Not sure if that the result we should expect.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 08 May 2024 12:52:35 AM UTC, comment #1: 

The heck?! I was testing with all kinds of inf and nan there and found that the nint_value was needed to specifically guard against inf so I left it in place with a comment about what it does. But I tested only with clang for both branches, not gcc.

Should I backout the changeset so that gcc will build it?

Arun Giridhar <arungiridhar>
Group Member
Wed 08 May 2024 12:48:03 AM UTC, original submission:  

Both with current stable and dev versions with gcc compilers,
clang seems to be OK?!)


octave:1> sort([1,2;3,4], inf)
--Type <RET> for more, q to quit, c to continue without paging--c

Thread 9 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffed4e83640 (LWP 391370)]
Array<double, std::pmr::polymorphic_allocator<double> >::sort (this=0x7ffec41a9668, dim=2147483647, mode=ASCENDING) at ../liboctave/array/Array-base.cc:1796
1796          octave_idx_type ns = dv(dim);
(gdb)


Full trace is attached.

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>

 

Attached Files

Attached Files
file #56023:  sort_coredump.txt.gz added by dasergatskov (3.4KiB - application/gzip)

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

Attach Files:
   
   
Comment:
   

 

Dependencies

This item does not depend on any other items.

No items depend on this one.

 

Mail Notification Carbon-Copy List

Carbon-Copy List
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by dasergatskov (Submitted the item)
  •  

    Votes

    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.

     

    History

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-05-12 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.2.0
        Planned ReleaseNone 10.1.0
    2024-05-08 nrjank Operating SystemGNU/Linux Any
    2024-05-08 arungiridhar StatusNone Ready For Test
    2024-05-08 dasergatskov Attached File- Added sort_coredump.txt.gz, #56023

    Back to the top

    Powered by Savane 3.16.
    Corresponding source code