tbeu / matio

MATLAB MAT File I/O Library
https://matio.sourceforge.io
BSD 2-Clause "Simplified" License
330 stars 97 forks source link

Mat_GetDir takes an infinite time to return values #157

Closed Nelson-numerical-software closed 8 months ago

Nelson-numerical-software commented 3 years ago

tested on windows matio 1.5.18 test_suites.zip

  char** variableNames = Mat_GetDir(matfile, &nVars);

mat contains a struct 'B' with many others struct as field.

Nelson-numerical-software commented 3 years ago

@tbeu : any advice (workaround) to speed up reading of this file ?

tbeu commented 3 years ago

Can reproduce. Mat_GetDir is poorly implemented and leads to performance loss for structs/cells.

jnjaeschke commented 3 years ago

Similar (?) thing here. Using matio 1.5.18 (current master) on windows in python (using ctypes).

As it turns out, I have different versions of Visual Studio on my machine and on our CI server (thanks, VS Update...): My Machine: -- The C compiler identification is MSVC 19.27.29112.0 -- The CXX compiler identification is MSVC 19.27.29112.0 CI: -- The C compiler identification is MSVC 19.24.28319.0 -- The CXX compiler identification is MSVC 19.24.28319.0

When calling Mat_GetDir on my machine (using the newer compiler), everything works as expected. Some Unit tests that call Mat_GetDir work as expected. On the CI server on the other hand (same build script, same Unit test) the function starts an infinite loop. By building with Debug information (RelWithDebInfo) I could find out that this loop does not stop:

            mat->num_datasets = 0;
            do {
                matvar = Mat_VarReadNextInfo(mat);
                if ( NULL != matvar ) {
                    if ( NULL != matvar->name ) {
                        if ( NULL == mat->dir ) {
                            dir = (char**)malloc(sizeof(char*));
                        } else {
                            dir = (char**)realloc(mat->dir,
                                (mat->num_datasets + 1)*(sizeof(char*)));
                        }
                        if ( NULL != dir ) {
                            mat->dir = dir;
                            mat->dir[mat->num_datasets++] = strdup(matvar->name);
                        } else {
                            Mat_Critical("Couldn't allocate memory for the directory");
                            break;
                        }
                    }
                    Mat_VarFree(matvar);
                } else if ( !feof((FILE *)mat->fp) ) {
                    Mat_Critical("An error occurred in reading the MAT file");
                    break;
                }
            } while ( !feof((FILE *)mat->fp) );

Mat_VarReadNextInfo seems to not stop here. This only occurs in 64bit.

As I said, updating the MSVC compiler seems to solve this...

tbeu commented 3 years ago

@jnjaeschke I cannot image that the difference in behaviour is due to the difference in compiler version. Could you please provide your MAT file, too?

jnjaeschke commented 3 years ago

Matio_issue_157_compiler_versions.zip

@tbeu In the zip file you find the test file (I think it was created using matio a while ago) as well as a working matio DLL (RelWithDebInfo, built on my machine) and the not working matio DLL built with the older compiler, also in RelWithDebInfo. The binaries are x64 and built using CMake.

I also noted a difference in file size (250kb <-> 251kb @ RelWithDebInfo, 159 <-> 162 kb @ Release), so there definitely is a difference...

Edit: Can confirm it is not a compiler problem. CI is updated to 19.27.xxx and the problem still occurs. Also only using the file I uploaded in the zip file... other files seem to work.

tbeu commented 3 years ago

@jnjaeschke Your reported issue is different from the original issue reported by @Nelson-numerical-software. Whereas the original issue is a performance problem of large and nested structures (of v5 or v7.3 MAT files), your issue (of v4 MAT files) is more critical and can lead to undefined behaviour (such as infinite loops). Please confirm if 289c586 fixes your reported issue in your environments. I will need to release a new version afterwards in case of positive feedback. Thanks.

jnjaeschke commented 3 years ago

@tbeu That fixed the issue. Thank you very much!

tbeu commented 8 months ago

Fixed by 274dfef.