bugGNU Octave - Bugs: bug #54299, HDF5 file I/O unit tests fail or...

 
 

bug #54299: HDF5 file I/O unit tests fail or crash Octave

Submitter:  Mike Miller <mtmiller>
Submitted:  Fri 13 Jul 2018 01:26:09 AM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  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

Fri 13 Jul 2018 09:01:49 PM UTC, comment #14: 

I pushed the following changeset to add comments based on what was written in https://savannah.gnu.org/bugs/index.php?49118#comment

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

I'm closing this report as fixed.  If there are any other issues, please open separate reports specifically about them.

I don't know what is best for the question in comment #10.  Do whatever you think is right.

John W. Eaton <jwe>
Group administrator
Fri 13 Jul 2018 08:29:56 PM UTC, comment #13: 

The change pushed on the default branch so far has fixed the original issue reported here.

Does this report need to stay open for further fixes related to this issue or can we close it as fixed?

Mike Miller <mtmiller>
Group Member
Fri 13 Jul 2018 05:29:05 PM UTC, comment #12: 

For a hopefully more complete explanation why I added get_ASCII_filename in the first place, please see:
https://savannah.gnu.org/bugs/index.php?49118#comment69

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jul 2018 05:23:51 PM UTC, comment #11: 

Erratum: " read from" in the previous comment should be "read or write an existing file in"

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jul 2018 05:16:15 PM UTC, comment #10: 

Sorry. I missed these.
The function get_ASCII_filename can help if a function relying on the char WinAPI (MS calls that the ANSI WinAPI) tries to read from a location that is only accessible with the wchar WinAPI (Unicode WinAPI in MS speak). It cannot help if a function relying on the ANSI WinAPI tries to create a file in a location that is only accessible with the Unicode WinAPI.

The FIXME outlines a way that might work for these file locations with H5Fcreate. However, I couldn't figure out how I could trigger these parts of the code. So I left it with the FIXME.
With your change H5Fcreate might create the file in a wrong location or with a wrong name.
I now realize that a possible test is:

a = 5;
save täst.mat a -hdf5


I might have a look at this later (different bug).


With respect to the other similar change in load-save.cc:

@@ -319,11 +323,11 @@ get_file_format (const std::string& fnam
     return LS_HDF5;
 #endif

 #if defined (HAVE_ZLIB)
-  use_zlib = check_gzip_magic (fname);
+  use_zlib = check_gzip_magic (ascii_fname);
 #else
   use_zlib = false;
 #endif

   if (! use_zlib)
     {


I tried to do the "conversion" as close as possible to the actual operation on the file system. So I did the "conversion" to an ASCII file name inside "check_gzip_magic". Since this is the only place where "check_gzip_magic" is called, we could remove "get_ASCII_filename" from there and do the above change instead. What is your opinion?


In the meantime I pushed the rest of your changes here to fix the immediate issue:
http://hg.savannah.gnu.org/hgweb/octave/rev/ca413f326224

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jul 2018 03:24:43 PM UTC, comment #9: 

In any case, could someone check my patch and commit?

John W. Eaton <jwe>
Group administrator
Fri 13 Jul 2018 03:23:40 PM UTC, comment #8: 

Markus:

In my patch, I changed




  const char *s_name =
     octave::sys::get_ASCII_filename (std::string (name)).c_str ();

  if (mode & std::ios::in)
    file_id = H5Fopen (s_name, H5F_ACC_RDONLY, octave_H5P_DEFAULT);
  else if (mode & std::ios::out)
    {
      if (mode & std::ios::app && H5Fis_hdf5 (s_name) > 0)
        file_id = H5Fopen (s_name, H5F_ACC_RDWR, octave_H5P_DEFAULT);
      else
        // FIXME: For Windows, create a file with an ASCII name in an
        //        accessible folder, close the file move and rename using
        //        wide character API and re-open.
        file_id = H5Fcreate (name, H5F_ACC_TRUNC, octave_H5P_DEFAULT,
                             octave_H5P_DEFAULT);
    }


to


  if (mode & std::ios::in)
    file_id = H5Fopen (s_name, H5F_ACC_RDONLY, octave_H5P_DEFAULT);
  else if (mode & std::ios::out)
    {
      if (mode & std::ios::app && H5Fis_hdf5 (s_name) > 0)
        file_id = H5Fopen (s_name, H5F_ACC_RDWR, octave_H5P_DEFAULT);
      else
        // FIXME: For Windows, create a file with an ASCII name in an
        //        accessible folder, close the file move and rename using
        //        wide character API and re-open.
        file_id = H5Fcreate (s_name, H5F_ACC_TRUNC, octave_H5P_DEFAULT,
                             octave_H5P_DEFAULT);
    }


This is the kind of thing I was asking about in comment #4.  Why was S_NAME used in the call to H5Fopen, but NAME used in the call to H5Fcreate?

I know I'm probably partly to blame for these bad tmp names, but it would probably also help if we consistently used more meaningful variable names here.  So instead of s_name, wchar_name or ascii_name or similar.  Then it might be more obvious from the code what is happening.  I can't quickly understand what that FIXME comment means, for example.

This code appears twice (copy and paste, apparently).  Probably a common function to do this job would also help with understanding  and future maintenance.

John W. Eaton <jwe>
Group administrator
Fri 13 Jul 2018 10:06:39 AM UTC, comment #7: 

It looks like the lifetime of the temporary std::string or the char pointer returned by c_str is compiler(?) dependent. jwe's diff looks good to me and since it also fixes the original problem, that change should be made. Thank you for coming up with the right idea so quickly.

It shouldn't cause problems using the original string later on in the code because both file paths should be pointing to the exact same file on disc. In fact imho, wherever it is possible the original file name should be used instead of a (possibly cryptic) name of a hard link to that file.

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jul 2018 06:23:37 AM UTC, comment #6: 

Lots of feedback to respond to here.

Markus - Sorry, this is on Debian GNU/Linux with GCC 8 and HDF5 1.10.0.

jwe - Yeah, your patch compiles without any problems and fixes all of the test failures for me in io.tst and sparse.tst.

Dmitri - I think the 'HDF5_18' configuration variable means that HDF5 is 1.8 or higher. There must have been an important API change between 1.6 and 1.8 that needed to be conditionally compiled.

Mike Miller <mtmiller>
Group Member
Fri 13 Jul 2018 05:23:36 AM UTC, comment #5: 

I just compiled a recent dev tip against a self-compiled hdf5-1.10.2 and I do not see the problem...
_octave_config_info_ reports that  HDF5_18 = 1 -- not sure what does it mean, may be i am doing something wrong.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 13 Jul 2018 04:30:51 AM UTC, comment #4: 

This looks like it might be a problem with the lifetime of temporary variables?

Does the attached patch help?

I changed all instances of


octave::sys::get_ASCII_filename (ff).c_str ()


to store the result of get_ASCII_filename in a temporary variable first.  However, it may only be the cases where you have something like


some_function (const char *name)
{
  ...
  octave::sys::get_ASCII_filename (std::string (name)).c_str ()
  ...
}


that are actually problems.  However, on Windows systems, you may be returning a different string from the one that is passed in.  What is the lifetime of that variable?  Does it always exist as long as the char pointer you get from the call to c_str?

In any case, I find it clearer to have the temporary ascii_fname variable made explicit.

Also, there are some instances of transforming the original name to the ascii name, then using the ascii name in a stream constructor or HDF5 function and then also using the original string in a later function.  My patch change a couple of those, but I'm not sure I did the right thing, and I didn't do an exhaustive check in all the functions that use get_ASCCI_filename.  We should probably carefully check them all, and if there are places where the ascii name AND the original name should both be used after the transformation, a comment explaining why they are both needed should be added.

+get_ASCII_

(file #44547)

John W. Eaton <jwe>
Group administrator
Fri 13 Jul 2018 03:40:47 AM UTC, comment #3: 

Is this on Linux or MacOS? The function "get_ASCII_filename" is supposed to do nothing with the input and just return what it received on non-Windows platforms:
https://hg.savannah.gnu.org/hgweb/octave/rev/7dad5fa7e88e

Am I missing something obvious?

Markus Mützel <mmuetzel>
Group administrator
Fri 13 Jul 2018 02:09:21 AM UTC, comment #2: 

Adding Markus in cc.

I ran hg bisect and it tells me this bug was first introduced with changeset

https://hg.savannah.gnu.org/hgweb/octave/rev/64715551b515

Markus - did you test this with HDF5 1.8 or 1.10 or both?

I have only HDF5 1.10 available on my system, and this change causes the tests to start failing with garbage file names as you can see in the test output snippet.

Mike Miller <mtmiller>
Group Member
Fri 13 Jul 2018 01:58:17 AM UTC, comment #1: 

Could it be HDF5 -1.10 issue? Fedora's buildbot has 1.8.20 and seems to be passing.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 13 Jul 2018 01:26:09 AM UTC, original submission:  

On the default branch, I get failed tests with HDF5 file I/O in io.tst and sparse.tst.

Example 'make check' output


...
  sparse.tst ..................................................HDF5-DIAG: Error detected in HDF5 (1.10.0-patch1) thread 140373378037504:
  #000: ../../../src/H5F.c line 579 in H5Fopen(): unable to open file
    major: File accessibilty
    minor: Unable to open file
  #001: ../../../src/H5Fint.c line 1100 in H5F_open(): unable to open file: time = Thu Jul 12 17:51:49 2018
, name = '�w�:�', tent_flags = 0
    major: File accessibilty
    minor: Unable to open file
  #002: ../../../src/H5FD.c line 812 in H5FD_open(): open failed
    major: Virtual File Layer
    minor: Unable to initialize object
  #003: ../../../src/H5FDsec2.c line 348 in H5FD_sec2_open(): unable to open file: name = '�w�:�', errno = 2, error message = 'No such file or directory', flags = 0, o_flags = 0
    major: File accessibilty
    minor: Unable to open file
HDF5-DIAG: Error detected in HDF5 (1.10.0-patch1) thread 140373378037504:
  #000: ../../../src/H5F.c line 579 in H5Fopen(): unable to open file
    major: File accessibilty
    minor: Unable to open file
  #001: ../../../src/H5Fint.c line 1100 in H5F_open(): unable to open file: time = Thu Jul 12 17:51:50 2018
, name = ' �S:�', tent_flags = 0
    major: File accessibilty
    minor: Unable to open file
  #002: ../../../src/H5FD.c line 812 in H5FD_open(): open failed
    major: Virtual File Layer
    minor: Unable to initialize object
  #003: ../../../src/H5FDsec2.c line 348 in H5FD_sec2_open(): unable to open file: name = ' �S:�', errno = 2, error message = 'No such file or directory', flags = 0, o_flags = 0
    major: File accessibilty
    minor: Unable to open file
 PASS   1202/1204
                                                                  FAIL    2
...
  io.tst ......................................................HDF5-DIAG: Error detected in HDF5 (1.10.0-patch1) thread 140373378037504:
  #000: ../../../src/H5F.c line 579 in H5Fopen(): unable to open file
    major: File accessibilty
    minor: Unable to open file
  #001: ../../../src/H5Fint.c line 1100 in H5F_open(): unable to open file: time = Thu Jul 12 17:52:19 2018
, name = '�"�9�', tent_flags = 0
    major: File accessibilty
    minor: Unable to open file
  #002: ../../../src/H5FD.c line 812 in H5FD_open(): open failed
    major: Virtual File Layer
    minor: Unable to initialize object
  #003: ../../../src/H5FDsec2.c line 348 in H5FD_sec2_open(): unable to open file: name = '�"�9�', errno = 2, error message = 'No such file or directory', flags = 0, o_flags = 0
    major: File accessibilty
    minor: Unable to open file
 PASS    156/157
                                                                  FAIL    1


I have also seen this error crash Octave, but I can't reproduce it at the moment.

I am not yet sure when this started failing for me, since I haven't been building the default branch regularly, but the stable branch does not have this problem at all.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44547:  diffs.txt added by jwe (9KiB - 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 jwe (Updated the item)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by mtmiller
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by mtmiller (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-13 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-07-13 mmuetzel StatusPatch Submitted Ready For Test
    2018-07-13 mtmiller StatusNone Patch Submitted
    2018-07-13 jwe Attached File- Added diffs.txt, #44547
    2018-07-13 mtmiller Carbon-Copy- Added mmuetzel

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code