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

 
 

bug #51530: fsanitize=undefined errors

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Fri 21 Jul 2017 07:33:45 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Dmitri A. Sergatskov Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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>
Group 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>
Group 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.


    static bool is_big_endian (void)
    {
      // Are we little or big endian?  From Harbison & Steele.

      union
      {
        long l;
        char c[sizeof (long)];
      } u;

      u.l = 1;

      return (u.c[sizeof (long) - 1] == 1);
    }


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):


The union is only as big as necessary to hold its largest data member. The other data members are allocated in the same bytes as part of that largest member. The details of that allocation are implementation-defined, and it's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group 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>
Group administrator
Mon 24 Jul 2017 12:24:42 AM UTC, comment #8: 

Great.  I'm closing this report.

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

That works too.


octave:1> test liboctave/util/oct-inttypes.cc-tst
PASSES 28 out of 28 tests
octave:2> test test/sparse.tst
PASSES 1204 out of 1204 tests
octave:3> __octave_config_info__ ("CXXFLAGS")
ans = -O0 -fsanitize=undefined
octave:4> __octave_config_info__ ("hg_id")
ans = f864950f837c


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>
Group 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:

  HG ID for this build is "203d3676730f+"

octave:1>  test liboctave/util/oct-inttypes.cc-tst
PASSES 28 out of 28 tests
octave:2> test test/sparse.tst
PASSES 1204 out of 1204 tests
octave:3> __octave_config_info__ ("CXXFLAGS")
ans = -O0 -fsanitize=undefined
octave:4>


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>
Group 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>
Group 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;


configure --disable-docs --without-qt --without-fltk CFLAGS=&quot;-O0 -fsanitize=undefined&quot; CXXFLAGS=&quot;-O0 -fsanitize=undefined&quot;


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


&lt;...&gt;
 gcc_ubsan/liboctave/util/oct-inttypes.cc-tst
................../liboctave/util/oct-inttypes.cc:431:40: runtime error:
negation of -9223372036854775808 cannot be represented in type 'long int';
cast to an unsigned type to negate this value to itself
 PASS     28/28
&lt;...&gt;
Fixed test scripts:

  bc-overloads.tst ............................................ PASS    451/451
  conv.tst .................................................... PASS    405/405
  sparse.tst ..................................................../libinterp/corefcn/ls-mat5.cc:492:11: runtime error: load of misaligned address 0x7f86637b5257 for type 'const int16_t', which requires 2 byte alignment
0x7f86637b5257: note: pointer points here
 68 2e 68 00 00  01 00 00 00 00 00 00 00  6c 6f 61 64 3a 20 65 72  72 6f 72 20 70 72 6f 62  69 6e 67
             ^
 PASS   1204/1204
&lt;...&gt;


Dmitri.
--

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

HG ID for this build is "cda0614beaec"

configure --disable-docs --without-qt --without-fltk CFLAGS="-O0 -fsanitize=undefined" CXXFLAGS="-O0 -fsanitize=undefined"
-verbatim
running "make check" results in two runtime errors:
+verbatim+
 gcc_ubsan/liboctave/util/oct-inttypes.cc-tst ................../liboctave/util/oct-inttypes.cc:431:40: runtime error: negation of -9223372036854775808 cannot be represented in type 'long int'; cast to an unsigned type to negate this value to itself
 PASS     28/28


Fixed test scripts:

  bc-overloads.tst ............................................ PASS    451/451
  conv.tst .................................................... PASS    405/405
  sparse.tst ..................................................../libinterp/corefcn/ls-mat5.cc:492:11: runtime error: load of misaligned address 0x7f86637b5257 for type 'const int16_t', which requires 2 byte alignment
0x7f86637b5257: note: pointer points here
 68 2e 68 00 00  01 00 00 00 00 00 00 00  6c 6f 61 64 3a 20 65 72  72 6f 72 20 70 72 6f 62  69 6e 67
             ^
 PASS   1204/1204


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by dasergatskov (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-24 jwe Attached File- Added endian-diffs.tst, #41317
    2017-07-24 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-07-23 rik5 Attached File- Added align.diff, #41294
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code