bugGNU Octave - Bugs: bug #51530, fsanitize=undefined errors

 
 

bug #51530: fsanitize=undefined errors

Submitted by:  Dmitri A. Sergatskov <dasergatskov>
Submitted on:  Fri 21 Jul 2017 07:33:45 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Segfault, Bus Error, etc.
Status: FixedAssigned to: None
Originator Name: Dmitri A. SergatskovOpen/Closed: Closed
Release: devOperating 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 Jump to the original submission)

Tue 01 Aug 2017 06:32:25 PM UTC, comment #19:

Rik: I'll make this change.

John W. Eaton <jwe>
Project Administrator
Tue 01 Aug 2017 06:21:35 PM UTC, comment #18:

I took a closer look at mach-info.cpp and more than one routine is using the unguaranteed union behavior. So, rather than convert the whole thing to C++ it probably does make sense to compile the file with a straight 'C' compiler. I don't know what that means for the use of namespaces which are in the file. I filed a new bug report to keep track of it.

Rik <rik5>
Project Administrator
Wed 26 Jul 2017 11:46:02 PM UTC, comment #17:

It seems clumsy to leave C++ and change the build system. Why not change mach-info.cpp to use an accepted method in C++?

Rik <rik5>
Project Administrator
Mon 24 Jul 2017 05:38:47 PM UTC, comment #16:

Maybe I should just move the is_big_endian and get_float_format functions to a C-language file?

John W. Eaton <jwe>
Project Administrator
Mon 24 Jul 2017 05:23:55 PM UTC, comment #15:

There is a problem with at least some of the code in mach-info.cc.

The code sets one member of a union, and then reads from another. In C, this behavior is guaranteed. In C++, this is not guaranteed. From cppreference.com (http://en.cppreference.com/w/cpp/language/union):

For a much longer article corroborating this, and where I got the idea for using memcpy, see http://dbp-consulting.com/tutorials/StrictAliasing.html.

Rik <rik5>
Project Administrator
Mon 24 Jul 2017 04:54:51 PM UTC, comment #14:

Yeah, no problem. I checked in a changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/e263cf468abd

John W. Eaton <jwe>
Project Administrator
Mon 24 Jul 2017 04:42:45 PM UTC, comment #13:

@jwe: I was feeling lazy so I thought I would just ask the question rather than troll through the source. It seems perfectly reasonable to use the code from comment #10 in place of what I wrote.

Rik <rik5>
Project Administrator
Mon 24 Jul 2017 04:34:06 PM UTC, comment #12:

Rik: It is initialized at run time the first time it is needed. See the get_float_format function in liboctve/system/mach-info.cc and the other functions there that use it.

John W. Eaton <jwe>
Project Administrator
Mon 24 Jul 2017 04:27:14 PM UTC, comment #11:

@jwe: I think your cset from comment #10 will work.

Is octave::mach_info::native_float_format () determined at compile-time or runtime? We need runtime evaluation because when cross-building with MXE the compile-time platform may have different endianness than the target.

Rik <rik5>
Project Administrator
Mon 24 Jul 2017 03:56:45 PM UTC, comment #10:

If I understand the intent of this code correctly, then I think the change would be as in the attached file. If so, then this seems clearer than using memcpy to set bytes.

(file #41317)

John W. Eaton <jwe>
Project Administrator
Mon 24 Jul 2017 03:43:10 PM UTC, comment #9:

Sorry to arrive a bit late on this one, but if these tests were just about determining endianness, could we use the functions from octave::mach_info instead?

John W. Eaton <jwe>
Project Administrator
Mon 24 Jul 2017 12:24:42 AM UTC, comment #8:

Great. I'm closing this report.

Rik <rik5>
Project Administrator
Sun 23 Jul 2017 07:55:17 PM UTC, comment #7:

That works too.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 23 Jul 2017 05:36:58 PM UTC, comment #6:

Detecting endianness turns out to be a difficult problem in C++. I checked in a different solution which should avoid the align warning from sanitize (http://hg.savannah.gnu.org/hgweb/octave/rev/f864950f837c).

Rik <rik5>
Project Administrator
Sun 23 Jul 2017 01:50:39 AM UTC, comment #5:

make check runs w/o problem.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 23 Jul 2017 01:20:56 AM UTC, comment #4:

Both problems appears fixed:

I am running full make check.

Dmitri.

Dmitri A. Sergatskov <dasergatskov>
Sun 23 Jul 2017 01:06:22 AM UTC, comment #3:

I can't reproduce the second issue, but it is probably because I have an older gcc. Can you try the attached patch align.diff. I've made a guess about what is wrong and I think this should fix it.

(file #41294)

Rik <rik5>
Project Administrator
Sun 23 Jul 2017 12:34:20 AM UTC, comment #2:

I fixed the first problem detected in oct-inttypes.cc. See cset http://hg.savannah.gnu.org/hgweb/octave/rev/203d3676730f.

Rik <rik5>
Project Administrator
Fri 21 Jul 2017 07:41:51 PM UTC, comment #1:

My &quot;verbatim&quot;s got messed up.
Trying again:

HG ID for this build is &quot;cda0614beaec&quot;

running &quot;make check&quot; results in two runtime errors:

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 21 Jul 2017 07:33:45 PM UTC, original submission:

HG ID for this build is "cda0614beaec"

Dmitri A. Sergatskov <dasergatskov>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #41317:  endian-diffs.tst added by jwe (889B - application/octet-stream)
file #41294:  align.diff added by rik5 (563B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by dasergatskov (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 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 24 Jul 2017 03:56:45 PM UTCjweAttached File-=>Added endian-diffs.tst, #41317
    Mon 24 Jul 2017 12:24:42 AM UTCrik5StatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Sun 23 Jul 2017 01:06:22 AM UTCrik5Attached File-=>Added align.diff, #41294
      StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1