bugGNU Octave - Bugs: bug #50234, c++ file_stat with non existant...

 
 

bug #50234: c++ file_stat with non existant crashes octave in windows

Submitted by:  John Donoghue <lostbard>
Submitted on:  Mon 06 Feb 2017 01:45:10 AM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: JohnDOpen/Closed: Closed
Release: 4.2.0Operating System: Microsoft Windows

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 Jump to the original submission)

 Spam posted by malkovich1985
Fri 24 Feb 2017 07:45:58 PM UTC, comment #26:

I pushed the change on the stable branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/a6e4157694ef). Unfortunately, this missed the 4.2.1 bug fix release by a few hours. I suppose this means anyone compiling 4.2.1 on Mac OS is going to need to apply this patch first.

Rik <rik5>
Project Administrator
Fri 24 Feb 2017 07:31:43 PM UTC, comment #25:

Rik, your new patch fixed the problem for me.

Ben Abbott <bpabbott>
Project Member
Fri 24 Feb 2017 07:06:25 PM UTC, comment #24:

Seems like your C++ linker can't find the compiled code for the ~file_stat() function. This may be due to over-aggresive optimization (the function is empty, and specified as being "inline", so clang may not be including it in the library of compiled code). Try the attached patch which gets rid of the inline keyword on the file_stat destructor.

(file #39823)

Rik <rik5>
Project Administrator
Fri 24 Feb 2017 06:55:00 PM UTC, comment #23:

Rik/John, I'm seeing the error below on MacOS.

My understanding of c++ is insufficient for fixing the problem. Can one of you propose a fix?

Ben Abbott <bpabbott>
Project Member
Mon 20 Feb 2017 05:12:33 AM UTC, comment #22:

I essentially checked in your changeset along with some comments explaining reminding any future programmers not to move these functions back to the header file and citing this bug report (http://hg.savannah.gnu.org/hgweb/octave/rev/525c36d5f985).

Rik <rik5>
Project Administrator
Thu 16 Feb 2017 05:25:42 PM UTC, comment #21:

For reference, I also tried the file_stat code as original, but with the file_name being set in the constructor body with same bad result

John Donoghue <lostbard>
Project Member
Thu 16 Feb 2017 05:19:39 PM UTC, comment #20:

Ok. I'm looking at whether some other changes to the constructor and destructor for file_stat might prevent this. But it takes an hour or so to test each iteration through MXE so maybe by the end of the day I might have something.

Rik <rik5>
Project Administrator
Thu 16 Feb 2017 05:03:43 PM UTC, comment #19:

I got gcc compiled ok and built a fresh install. I few minor changes were needed to Qt and wxwidgets to get them to compile

Running in windows however, the same code still crashes

John Donoghue <lostbard>
Project Member
Tue 14 Feb 2017 11:38:49 PM UTC, comment #18:

Ive got some changes now that compiled build-gcc 6.3.0 ok - its currently compiling native-gcc, so may work.

To get a --with-gcc work, we may need to implement something similar to the plugins that mxe.cc is using and would be better for testing different versions at least.

John Donoghue <lostbard>
Project Member
Tue 14 Feb 2017 10:23:38 PM UTC, comment #17:

I'm willing to work on newer compiler versions.

Would it be worthwhile to add an option like --with-gcc=VERSION? I've considered it in the past but haven't had time to do it.

John W. Eaton <jwe>
Project Administrator
Tue 14 Feb 2017 07:00:22 PM UTC, comment #16:

For comment #13, removing the 'virtual' and it still works

For comment #15, we should be able to try a new compiler although it may take a while to get it working

John Donoghue <lostbard>
Project Member
Tue 14 Feb 2017 04:46:03 PM UTC, comment #15:

Can we try a newer version of gcc to check if it is the compiler, rather than the code, which is at fault? Aside from the point about the reference in comment #10, this looks like valid C++ code.

Rik <rik5>
Project Administrator
Tue 14 Feb 2017 11:14:12 AM UTC, comment #14:

Both are needed or it still crashes.

I hadnt tried without the virtual so will try that.

This is all using 4.9.4. We could try gcc 5 or 6 ? and see if there is other any additional issues that show up

John Donoghue <lostbard>
Project Member
Tue 14 Feb 2017 03:39:42 AM UTC, comment #13:

I don't object to the patch, but since the destructor is trivial and the constructor is pretty simple and many constructors of even greater complexity are defined in header files, we should probably add a note about why these are defined in the .cc file.

Also, do both need to be moved?

Is the virtual declaration for the destructor required?

What version of GCC? Is it 4.9.4? Should we update the version used by mxe-octave or does that cause other trouble?

John W. Eaton <jwe>
Project Administrator
Mon 13 Feb 2017 09:52:46 PM UTC, comment #12:

This patch (attached) appears to work for me - not sure why its an issue otherwise.

(file #39750)

John Donoghue <lostbard>
Project Member
Fri 10 Feb 2017 04:28:32 PM UTC, comment #11:

suggested change appears to make no difference.

John Donoghue <lostbard>
Project Member
Thu 09 Feb 2017 05:56:03 PM UTC, comment #10:

Confirmed. For completeness, I booted an old Windows XP VM and ran 4.2.0 to create a .oct file and it segfaults in the manner reported here.

Looking at the code in liboctave/system/file-ops.cc I see

I'm grasping here, but I see that name is passed in by reference, and that the function prototype declares this const which means it is making a promise not to change it. However, tilde_expand just returns a std::string object. When a '~' is found, the function declares a temporary "std::string result" and then proceeds to construct that string before ending with "return result;".

On the other hand, if no '~' is found then the function actually passes a reference to a string ("return name;") rather than a string object. It is possible that eventually the reference is invalid but downstream functions which called tilde_expand continue to try and use it.

As a quick test, one might try altering the return statement to

Rik <rik5>
Project Administrator
Thu 09 Feb 2017 12:37:12 AM UTC, comment #9:

I tried that after I first saw it just on an offchance, but the error (and stack trace) was the same

John Donoghue <lostbard>
Project Member
Thu 09 Feb 2017 12:21:28 AM UTC, comment #8:

Just out of curiosity, what happens if you rewrite it as

?

John W. Eaton <jwe>
Project Administrator
Thu 09 Feb 2017 12:06:59 AM UTC, comment #7:

After catching the error and looking in gdb, the name looks invalid in update_internal, however looks ok up a step so I think the stack is getting messed up.

Im retrying with some print statements in it

John Donoghue <lostbard>
Project Member
Wed 08 Feb 2017 11:40:31 PM UTC, comment #6:

The stack trace says that it is crashing in the call to find, here:

How? What is the value of name at this point?

John W. Eaton <jwe>
Project Administrator
Wed 08 Feb 2017 07:59:36 PM UTC, comment #5:

That change does not fix it for me however, that change also needs to be applied to stable as well

John Donoghue <lostbard>
Project Member
Tue 07 Feb 2017 06:58:31 PM UTC, comment #4:

I saw that - from the change, I don't think it will help but am trying a rebuild to verify.

John Donoghue <lostbard>
Project Member
Tue 07 Feb 2017 06:08:58 PM UTC, comment #3:

I don't know if it is related, but there was a check-in yesterday, Feb. 6th, that fixes a problem with the stat wrappers.

Can you try building an MXE version from the development sources with a cset >= 23159?

Rik <rik5>
Project Administrator
Tue 07 Feb 2017 05:25:35 PM UTC, comment #2:

Anyone else running windows able to see the same issue?

John Donoghue <lostbard>
Project Member
Mon 06 Feb 2017 01:47:48 AM UTC, comment #1:

On a similar run with symbols:

John Donoghue <lostbard>
Project Member
Mon 06 Feb 2017 01:45:10 AM UTC, original submission:

Using a oct file created in windows 7, octave 4.2.0 (tried both w64 and w32) versions

I get a consistent hard crash of octave running the function:

oct file built as mkoctfile notest.cpp

Run in octave as notest.

STacktrace without any debug symbols compiled in is:

John Donoghue <lostbard>
Project Member

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #39823:  inline.diff added by rik5 (396B - text/x-diff)
file #39750:  oct_no_crash.patch added by lostbard (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by malkovich1985 (Posted a comment)
  • -unavailable- added by bpabbott (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by lostbard (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 24 Feb 2017 07:45:58 PM UTCrik5Open/ClosedOpen=>Closed
    Fri 24 Feb 2017 07:06:25 PM UTCrik5Attached File-=>Added inline.diff, #39823
    Fri 24 Feb 2017 06:55:00 PM UTCbpabbottOpen/ClosedClosed=>Open
    Tue 21 Feb 2017 10:54:22 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Mon 20 Feb 2017 05:12:33 AM UTCrik5StatusConfirmed=>Ready For Test
    Mon 13 Feb 2017 09:52:46 PM UTClostbardAttached File-=>Added oct_no_crash.patch, #39750
    Thu 09 Feb 2017 05:56:03 PM UTCrik5StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1