bugGNU Octave - Bugs: bug #50102, dlmread crashing the interpreter...

 
 

bug #50102: dlmread crashing the interpreter on Cygwin

Submitter:  Julien K. <julien_k>
Submitted:  Fri 20 Jan 2017 12:29:31 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 06 Apr 2017 01:57:55 PM UTC, comment #30: 

Alas, some bugs are just like that.  I'll close the bug report and we'll hope that it doesn't return.

Rik <rik5>
Group administrator
Thu 06 Apr 2017 12:07:51 PM UTC, comment #29: 

Hi Rik,

I tested the mercurial changeset as you advised me and it stil crash with dlmread. I got fed up with this and wiped out my cygwin install and reinstalled it from scratch. Now it works...

Sorry for the inconvenience; still weird this bug did not occur with debug flags turned on.

Anyway thanks a lot for your work on Octave!

Regards,
Julien

Julien K. <julien_k>
Wed 22 Mar 2017 06:32:13 AM UTC, comment #28: 

Okay, I think this is ready for re-testing.  Please use a Mercurial changeset >= 23325:4adb9517d56a.

The last change was to handle large indices in the RANGE specification.  Because we were casting from double to int some large values could actually end up negative and mess things up.  See http://hg.savannah.gnu.org/hgweb/octave/rev/31be59137160.

Rik <rik5>
Group administrator
Wed 22 Mar 2017 03:46:29 AM UTC, comment #27: 

@J.K.: Please wait just another day or so.  I have yet another changeset that handles overly range inputs.  After that, I think it will be worth re-compiling with the latest development code because there have been a ton of changes to the code.

Rik <rik5>
Group administrator
Wed 22 Mar 2017 01:38:44 AM UTC, comment #26: 

FWIW, the error surrounding dlmread() seems to have gone away for what I am using it for.  I'm pretty sure I had isolated the error to csvread()--which is only a single instruction call to dlmread()--by surrounding that csvread() call with prints to the screen.  There have been a fair amount of changes in just the past week, so about all that can be said is that something appears to have been fixed.

J.K., if you are still following, when you get a chance, please try your original problematic code to see if the error has cleared.  If so, then we can close this bug report.

Dan Sebald <sebald>
Mon 20 Mar 2017 04:52:45 AM UTC, comment #25: 

Comment 23: The changeset looks good...tricky routine that one.

Comment 24: Any sort of exponential growth is perfectly fine.  I thought maybe it was intended to always be power of 2.

Dan Sebald <sebald>
Sun 19 Mar 2017 10:19:55 PM UTC, comment #24: 

We could add more code to keep rmax a perfect power of two, but I don't think it is really necessary.  The point is merely to avoid re-sizing the array for every single row added.  In the case you outlined, the progression of size would be 32, 64, 70, 140, 280, ..., versus 32, 64, 128, 256, ...

Essentially one extra callto resize would be issued, and it would be at a low value of rmax so the resize will be easy.


Rik <rik5>
Group administrator
Sun 19 Mar 2017 10:13:54 PM UTC, comment #23: 

The interpreter shouldn't produce an error for valid input.  I checked in a change on the stable branch that handles the case you discovered where specifying a column c0 which is greater than the number of actual columns of data emits an error.  See http://hg.savannah.gnu.org/hgweb/octave/rev/85ffe1bdd3a0.  Because it was really the same test, I also changed dlmread to return a 0x0 empty matrix when there is no data in the file.

Rik <rik5>
Group administrator
Sun 19 Mar 2017 09:03:28 PM UTC, comment #22: 

OK, improvement.  That test against 1 is sort of redundant in that the apriori condition on rmax is/was known.  The double expanding is the way to go.  I like that.  I see that you've chosen to not double expand the columns because that is estimated at the beginning line and typically won't keep expanding throughout the rest of the file (although it is allowed to). 

However, I think the dual tests on rmax and cmax simultaneously leads to this not always being what the comment indicates.  If I understand correctly,


          if (r > rmax || c > cmax)
            {


is meant to allow adjustments for column size as reading goes forward.  Let's say starting at row 35 is the following:

1,34,64
2,45,83
...
35,58,60
36,26,99,-3
37,53,78,-5
...

Because c expands by one at row 36, the above condition is going to test true.  So, at that point r is 36, and then there is an automatic readjustment even though it was the column size that changed:


              // Use resize_and_fill for the case of unequal length rows.
              // Keep rmax a power of 2.
              rmax = std::max (2*(r-1), rmax);
              cmax = std::max (c, cmax);


so rmax becomes 2*(r-1) = 70, not a power of two (if the power of two is preferred).  There are two ways of approaching this.  Minimizing the potential number of reshapes per pass:


          if (r > rmax || c > cmax)
            {
              if (r > rmax)
                  rmax = std::max (2*(r-1), rmax);
              cmax = std::max (c, cmax);
[reshape]


A second is minimizing a little extra code within the test:


          if (r > rmax)
            {
              rmax = std::max (2*(r-1), rmax);
[reshape]
            }
          if (c > cmax)
            {
              cmax = c;
[reshape]
            }


The benefit of one over the other is dependent on the statistical nature of the data and even then doesn't make much difference.  So, it boils down to preference.

Dan Sebald <sebald>
Sun 19 Mar 2017 08:29:25 PM UTC, comment #21: 

I checked in a simple change to have rmax increase in powers of 2 as it is supposed to.


-              rmax = 2 * std::max (r-1, static_cast<octave_idx_type> (1));
+              rmax = std::max (2*(r-1), rmax);


See http://hg.savannah.gnu.org/hgweb/octave/rev/a009a871825c.

Rik <rik5>
Group administrator
Sun 19 Mar 2017 05:02:31 PM UTC, comment #20: 

It is minor because it has no effect.  isempty() will return true, no functions, like sin(), will calculate anything because there are no numbers on which to calculate.  Matlab is very clear that the concept of "empty" does not mean 0x0 as humans think.  Their functions often return 1x0 or 0x1 or other odd combinations.

Rik <rik5>
Group administrator
Sun 19 Mar 2017 04:38:10 PM UTC, comment #19: 

"z = [](32x0)

It is a very minor nit, but it would be better to return a 0x0 empty matrix in this case."

Not so minor really.  The user will be clueless as to what the "32" means, being a sort of temporary red herring.

Dan Sebald <sebald>
Sun 19 Mar 2017 04:28:01 PM UTC, comment #18: 

OK.  It does look like space is made for the whole matrix before the assignments are made.

To point out my original theory about rmax going backward from 32, I printed out the value of rmax and cmax whenever they change:


diff --git a/libinterp/corefcn/dlmread.cc b/libinterp/corefcn/dlmread.cc
--- a/libinterp/corefcn/dlmread.cc
+++ b/libinterp/corefcn/dlmread.cc
@@ -274,7 +274,9 @@ such as text, are also replaced by the @
   octave_idx_type c = 1;
   // Start with a reasonable size to avoid constant resizing of matrix.
   octave_idx_type rmax = 32;
+fprintf(stderr, "rnamx = %ld\n", rmax);
   octave_idx_type cmax = 0;
+fprintf(stderr, "cnamx = %ld\n", cmax);

   Matrix rdata (rmax, cmax, empty_value);
   ComplexMatrix cdata;
@@ -406,7 +408,9 @@ such as text, are also replaced by the @
               // Use resize_and_fill for the case of unequal length rows.
               // Keep rmax a power of 2.
               rmax = 2 * std::max (r-1, static_cast<octave_idx_type> (1));
+fprintf(stderr, "rmax = %ld\n", rmax);
               cmax = std::max (c, cmax);
+fprintf(stderr, "cmax = %ld\n", cmax);
               if (iscmplx)
                 cdata.resize (rmax, cmax, empty_value);
               else


Here's the result for a simple 2 by 3 matrix:


octave:2> x = dlmread('mat2x3.csv')
rmax = 32
cmax = 0
rmax = 2
cmax = 3
x =

   1   2   3
   4   5   6


Again, not a bug, but it defeats the purpose of setting rmax to 32, which the comment says is to prevent those first bunch of resizes that happen so quickly as the matrix starts out.  In my opinion, I wouldn't be too worried if rmax started at 2 (initializing rmax = 0 will result in


    rmax = 2 * std::max (r-1, static_cast<octave_idx_type> (1));


rmax equal 2.  Those very first re-assignments, although coming quickly, are very small and don't consume much CPU.  It's the bigger resizing that might have to expand the matrix and copy a large hunk of memory.

I did find an inconsistency in behavior when choosing the starting indeces too large to have valid data.  If the row is too large, the result is empty matrix; if the column is too big, the result is an error:


octave:10> x = dlmread('mat2x3.csv',',',10,1)
x = [](32x0)
octave:11> x = dlmread('mat2x3.csv',',',1,10)
error: index (_,11): but object has size 2x3


Dan Sebald <sebald>
Sun 19 Mar 2017 02:56:50 PM UTC, comment #17: 

If no delimiter is found then cmax is not incremented; But this is the correct behavior for corner cases.  For example, create a text file with a few newlines and use dlmread on it.  It will return '[]' because the file was empty.  However if you increment cmax to 1 then it will return a '0' for each line because it thought there was at least one column of data when there really wasn't.

With a test file 'abc' of three newlines


octave:1> z = dlmread ('abc')
z = [](32x0)


It is a very minor nit, but it would be better to return a 0x0 empty matrix in this case.

Secondly, the resize function does accepts a zero as input without problem.  In the Octave interpreter,


octave:1> x = magic(3)
x =

   8   1   6
   3   5   7
   4   9   2

octave:2> resize (x, 3,0)
ans = [](3x0)



Rik <rik5>
Group administrator
Sun 19 Mar 2017 07:53:38 AM UTC, comment #16: 

Is it always true that cmax gets incremented?  There is a conditional there:


              // Separator followed by EOL doesn't generate extra column
              if (pos2 != std::string::npos)
                cmax++;
[snip]
            }
          while (pos2 != std::string::npos);


If pos2 equals -1 (i.e., std::string::npos), then cmax is not incremented and the while loop exits.

Let's say the file has only one column.  Then there is no "sep" to be found and cmax isn't incremented.  Is that possible?  (It's difficult to follow.)  And if that happens, then


          // FIXME: Should always be the case that iscmplx == false.
          //        Flag is initialized that way and no data has been read.
          if (iscmplx)
            cdata.resize (rmax, cmax, empty_value);
          else
            rdata.resize (rmax, cmax, empty_value);


where rmax = 32 and cmax = 0, which I wonder is valid.

Let me think about this, but just ask you if you think there should be something added like the following:


       // Estimate the number of columns from first line of data.
       if (cmax == 0)
         {
           size_t pos1, pos2;
           if (auto_sep_is_wspace)
             pos1 = line.find_first_not_of (" \t");
           else
             pos1 = 0;
+          cmax = 1;


That is, upon finding the first non-blank line there is automatically one column and then an additional column is added for every separator found.  In other words, cmax is the number of separators plus 1.

Dan Sebald <sebald>
Sun 19 Mar 2017 06:15:17 AM UTC, comment #15: 

@Dan: This statement is not accurate.


On the first pass, c > cmax will test true. So, right away, since r=1, rmax = 2 * max(0,1) = 2 such that rmax goes from 32 down to 2.



The code begins by pre-assigning the value of rmax, but not cmax.


  // Start with a reasonable size to avoid constant resizing of matrix.
  octave_idx_type rmax = 32;
  octave_idx_type cmax = 0;


Next, there is a block that attempts to find the number of columns in the data.


      // Estimate the number of columns from first line of data.
      if (cmax == 0)


The variable cmax is incremented during that process.

Only later are the number of columns and rows checked


          c = (c > j + 1 ? c : j + 1);
          if (r > rmax || c > cmax)
            {
              // Use resize_and_fill for the case of unequal length rows.
              // Keep rmax a power of 2.
              rmax = 2 * std::max (r-1, static_cast<octave_idx_type> (1));


The reason for the -1 is because the if () test is '>'.  The first time this is triggered, for example, r will be 33 (> 32).  Thus, to keep this to a power of 2 the code substracts one, and then multiplies by 2.

Without a failing example that can be traced through the debugger I fear not much progress can be made on this report.


Rik <rik5>
Group administrator
Mon 13 Mar 2017 04:52:11 PM UTC, comment #14: 

Also of note is that


if (r > rmax) {}
do {
  if (c > cmax) {}
  etc.
}


can reduce the CPU consumed because that r > rmax is redundant within the inner loop.  Not that big of a savings as there is plenty other instructions such as reading/saving the data.

Dan Sebald <sebald>
Mon 13 Mar 2017 04:32:09 PM UTC, comment #13: 

No hurry Rik, but I'd prefer you fix this one whenever you find the time.

There is something dodgy here, and I think that the following conditional might be too broad:



  // Start with a reasonable size to avoid constant resizing of matrix.
  octave_idx_type rmax = 32;
  octave_idx_type cmax = 0;
[snip]
          c = (c > j + 1 ? c : j + 1);
          if (r > rmax || c > cmax)
            {
              // Use resize_and_fill for the case of unequal length rows.
              // Keep rmax a power of 2.
              rmax = 2 * std::max (r-1, static_cast<octave_idx_type> (1));
              cmax = std::max (c, cmax);
              if (iscmplx)
                cdata.resize (rmax, cmax, empty_value);
              else
                rdata.resize (rmax, cmax, empty_value);
            }


On the first pass, c > cmax will test true.  So, right away, since r=1, rmax = 2 * max(0,1) = 2 such that rmax goes from 32 down to 2.

That in itself isn't a bug, as rmax will just keep incrementing again.  But I keep wondering if there is a scenario where if r = 2, then

rmax = max(r-1, 1)
= max(2-1,1)
= 2

I.e., the buffer doesn't expand when it is expected to.  So, I wonder if perhaps the single "if (r > rmax || c > cmax)" should be broken in "if (r > rmax)" and "if (c > cmax)" where in the first case the row size is readjusted and in the second case the column size is adjusted (and r > rmax test can be moved earlier).

Also, why not simply use the doubling-buffer-size rule for columns as well as rows?  The user could mistakenly, or otherwise, input a long data file for which there are only white space separators as opposed to new-line separators.  The former would be much slower than the latter.

Dan Sebald <sebald>
Mon 13 Mar 2017 03:26:18 PM UTC, comment #12: 

[I must have mistyped "verbatim"]

or, more likely, this


-              rmax = 2*r;
-              cmax = c;
+              rmax = 2 * std::max (r-1, 1);  // keep rmax a power of 2
+              cmax = std::max (c, cmax);


Why was the change made from 2*r to 2*(r-1)?  Was there an extra row because dlmread attempts to read into a "next" line not knowing if there would be one or not?

Dan Sebald <sebald>
Mon 13 Mar 2017 03:25:40 PM UTC, comment #11: 

Getting exact compatibility with matlab for dlmread was really difficult.  There were a lot of corner cases and 3 or 4 bugs filed by Philip about various aspects.  I'm pretty sure I don't want to dive in to that again.

What happens if you run with '--no-gui-libs' so you really disable the Qt libraries and then use FLTK or gnuplot for plotting?


-              rmax = 2*r;
-              cmax = c;
+              rmax = 2 * std::max (r-1, 1);  // keep rmax a power of 2
+              cmax = std::max (c, cmax);


I think the change may just have been something about 0-based and 1-based indexing.  But, I can't remember for sure.


Rik <rik5>
Group administrator
Mon 13 Mar 2017 09:27:19 AM UTC, comment #10: 

I'm going to add to the noise here because I have an obscure bug I can't make sense of and the only info is dlmread and munmap_chunk().  (This is linux, latest development code.)

I can't narrow things down because it happens under a rather complex script involving lots of data files and links.  If I try to replicate things with direct code on a specific file, the crash doesn't happen.

So, I'll just give the message before crash and see if it rings a bell for anyone:


warning: dlmread: '/home/sebald/data/foo.csv' found by searching load path
*** Error in `/usr/local/src/octave/octave/build3/src/.libs/lt-octave-gui': munmap_chunk(): invalid pointer: 0x00007f23549c2e78 ***
Aborted


Launched with:

/usr/local/src/octave/octave/build3/run-octave --no-gui

Why the error is in the gui library when launched with --no-gui, I don't know.  Maybe the bad pointer is somewhere else.  And what this munmap_chunk() is, I have no idea.  Must be a system library used either by Octave core or Qt.

The program crashes at a line that does csvread() which must call dlmread().  Also, this line of code is within a try-end block.

I have an older Octave version installed system wide and run via octave-cli that works fine.

The most recent dlmread-related changeset is

http://hg.savannah.gnu.org/hgweb/octave/rev/68d5c4759783

Rik, you know this better than I, as I'm just looking at the changeset.  If I were to investigate deeper, I'd probably start with checking this:


-              cmax++;
+              // Separator followed by EOL doesn't generate extra column
+              if (pos2 != std::string::npos)
+                cmax++;


or, more likely, this


-              rmax = 2*r;
-              cmax = c;
+              rmax = 2 * std::max (r-1, 1);  // keep rmax a power of 2
+              cmax = std::max (c, cmax);

Why was the change made from 2*r to 2*(r-1)?  Was there an extra row because dlmread attempts to read into a "next" line not knowing if there would be one or not?

Dan

Dan Sebald <sebald>
Mon 30 Jan 2017 02:29:04 PM UTC, comment #9: 

Hi Rik,

the situation's not as simple as I thought a few days ago:
- the octave build with debug options does not crash and passes 'make check',
- the normal build does crash (inc. 'make check'),
- the brand new 4.2.0 package from cygwin repository also crashes.

Could you please reopen the bug?

Regards,
Julien

Julien K. <julien_k>
Thu 26 Jan 2017 04:25:09 PM UTC, comment #8: 

This is still an okay result--no bug found in Octave and your back to working again.  Closing report.

Rik <rik5>
Group administrator
Thu 26 Jan 2017 08:21:50 AM UTC, comment #7: 

Hi

I compiled last night a debug-enabled version of octave-4.2, ran it through gdb and my dlmread test case now passes smoothly (and so is make check) :-/

There has been a few lib updates on the cygwin32 distribution in the past few days so the crash seems to have solve itself by recompiling...

This is embarrassing, I'm sorry for the noise. Thank you for your attention anyway!

Regards,
Julien

Julien K. <julien_k>
Tue 24 Jan 2017 04:50:12 PM UTC, comment #6: 

Did the command


dlmread ('A.txt', ';')


also segfault?  I thought that might help diagnose whether it was the separator detection part of the code that was having problems.

Since you are building from scratch you could use the '-g' flag to include debugging information and then run Octave under gdb to get a backtrace when the segfault happens.  That would at least point to where the code is failing.

Alternatively, if that sounds like a lot of work, maybe just pass on solving this bug and use the available Windows installer version of Octave that seems free of this problem.

Rik <rik5>
Group administrator
Tue 24 Jan 2017 04:45:25 PM UTC, comment #5: 

Oops, I just uploaded A2.txt.  Although it probably doesn't make a difference as you suggest.

(file #39549)

Rik <rik5>
Group administrator
Tue 24 Jan 2017 08:45:35 AM UTC, comment #4: 

Hi Rik,

I don't see your file but it's ok, I converted my A.txt in LF line ending. It still crashes octave.


$ od -c A2.txt
0000000   0   .   1   3   0   6   4   4   1   4   5   2   8   1   4   3
0000020   4   3   ;   0   .   8   8   5   7   7   6   7   3   8   5   5
0000040   5   9   9   2   5   ;   0   .   9   6   8   3   1   8   8   4
0000060   9   9   5   2   6   5   8   9   ;   0   .   7   0   9   8   4
0000100   6   1   2   3   9   8   0   2   8   4   2  \n   0   .   7   2
0000120   4   5   1   5   2   9   1   0   9   4   5   0   7   2   ;   0
0000140   .   1   7   3   4   3   8   4   1   7   6   3   7   0   1   1
0000160   ;   0   .   7   1   2   1   7   7   2   9   4   4   6   1   6
0000200   3   5   1   ;   0   .   2   5   4   7   9   7   7   8   7   8
$ ./run-octave
octave:1> dlmread('A2.txt')
panic: Segmentation fault -- stopping myself...
attempting to save variables to 'octave-workspace'...


Regards,
Julien

Julien K. <julien_k>
Mon 23 Jan 2017 04:37:47 PM UTC, comment #3: 

Just downloaded and tested your file A.txt with Octave 4.2.0 in a Windows XP virtual machine and it passed fine.

So it does seem to be a cygwin-specific, home-built issue.  I've uploaded A2.txt which is simply A.txt with the line ending changed from Windows to Unix.  Does this work for you?


dlmread ('A2.txt')


Also, if Octave is having trouble finding the delimiter, does this work


dlmread ('A.txt', ';')



Rik <rik5>
Group administrator
Mon 23 Jan 2017 08:35:19 AM UTC, comment #2: 

Hi Rik,

thanks for your message, here are my answers.

Please find the A.txt file attached.
I can add that "make check" fails (build of the octave-4.2.0 release source tree):

  libinterp/corefcn/det.cc-tst ............................ PASS      5/5
  libinterp/corefcn/dirfns.cc-tst .........................panic: Segmentation fault -- stopping myself...

The windows installer works:

>> A = rand(2,3) ;
>> dlmwrite('A.txt',A,';') ;
>> dlmread('A.txt')

ans =

   0.567935   0.198092   0.671918
   0.336125   0.448137   0.085711

So it is only a self-built problem on cygwin...

Regards,
Julien

(file #39538)

Julien K. <julien_k>
Sat 21 Jan 2017 11:12:42 PM UTC, comment #1: 

Can you attach the problem file so it can be tested?  It's not enough to list the content because it may be something about the end-of-line sequence (newline + carriage return for Windows, just newline for Linux).

Does the Windows Installer version of Octae 4.2.0 also have this issue (https://ftp.gnu.org/gnu/octave/windows/)?  Or is it only a self-built version?

Rik <rik5>
Group administrator
Fri 20 Jan 2017 12:29:31 PM UTC, original submission:  

Dear octave maintainers,

I built octave-4.2.0 from source on cygwin32 and I get a panic while trying to read a file

I run octave directly from the build dir as a test:

$ ./run-octave
[ ... ]
octave:1> A = rand(2,4) ;
octave:2> dlmwrite('A.txt',A,';') ;
octave:3> dlmread('A.txt',';') ;
panic: Segmentation fault -- stopping myself...
attempting to save variables to 'octave-workspace'...

The file "A.txt" seems ok:

$ cat A.txt
0.1306441452814343;0.8857767385559925;0.9683188499526589;0.7098461239802842
0.7245152910945072;0.173438417637011;0.7121772944616351;0.2547977878928745

My configure options are:

./configure --enable-shared --disable-64 --disable-docs \
  --disable-java --enable-threads=windows --enable-bounds-check

Am I doing anything wrong here? Is this a cygwin issue?

Regards,
Julien

Julien K. <julien_k>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39549:  A2.txt added by rik5 (151B - text/plain)
file #39538:  A.txt added by julien_k (153B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by julien_k (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-04-06 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-03-22 rik5 StatusNone Ready For Test
        Release4.2.0 dev
    2017-01-30 rik5 StatusInvalid / Not an Octave Bug None
        Open/ClosedClosed Open
        Summarycsvread/dlm read crashes the interpreter dlmread crashing the interpreter on Cygwin
    2017-01-26 rik5 StatusNeed Info Invalid / Not an Octave Bug
        Open/ClosedOpen Closed
    2017-01-24 rik5 Attached File- Added A2.txt, #39549
    2017-01-23 julien_k Attached File- Added A.txt, #39538
    2017-01-21 rik5 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code