Fix readLength() return type error

[Yida-Lin]

This is a bug that causes SigViewer to hang. The return type of the readLength() must be uint64_t. Implicitly casting it causes errors.

[Yida-Lin]

Btw, are the tag numbers and chunk length etc. in XDF format always non-negative? Would there occasionally be -1 for example? I didn't find enough details in the specs. Not sure about the use of uint during reading files; I would rather go safe with int8, int32 if possible - at least it can't go wrong.

[cbrnr]

Btw, are the tag numbers and chunk length etc. in XDF format always non-negative? Would there occasionally be -1 for example?

Yes, it's not mentioned explicitly, but the idea is that tag numbers start with 1 and increase. Chunk length cannot be less than zero, AFAIK we don't use any special indicator (such as -1) to flag something unusual.

[cbrnr]

LGTM!

[tstenner]

The chunk type is defined as uint16_t and the sizes are unsigned integer types.

The return type of readLength is already uint64_t so this should be unnecessary as the smaller unsigned values will be promoted to uint64_t either before storing them in the temporary variable or putting the return value on the stack. I rather suspect that there's a problem somewhere else that's shifted by the temporary variable.

Could you upload the problematic file somewhere or show in a debugger that a smaller type gets promoted incorrectly, i.e. the return value of readBin doesn't match the one from readLength?

[tstenner]

One additional thing: I've compiled the code with clang 9 (clang++ -Wall -Wpedantic -Wextra -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -c -Ipugixml/ xdf.cpp) and while there where 121 sign conversion (and 15 integer conversion) warnings, there were none in readBin.

[Yida-Lin]

@tstenner for me on a Win 10 machine opening any XDF file using SigViewer would hang. I already turned off my laptop so cannot show at the moment, but I made extensive tests earlier and am pretty sure this is the cause. I can provide some sample XDF files tomorrow if needed.

[Yida-Lin]

I use MinGW 5.3. Problem likely related to platform specific compiler.

[tstenner]

Does MinGW produce 64 bit binaries?

[cbrnr]

Does MinGW produce 64 bit binaries?

The version @Yida-Lin is using doesn't - or at least I assume, because we've been building SigViewer with the 32bit MinGW version ever since. Qt has started to ship a 64bit version only recently so I haven't made the switch yet.

[tstenner]

Then there will be fun type conversions from uint64_t to std::size_t (uint32_t) and std::streamsize (int32_t), even though those wouldn't make a difference for all lengths smaller than 2^32-1.

The Qt installer ships the 64 bit version of MinGW 7.3, so this could be a good time to switch.

[Yida-Lin]

@tstenner Good point, this sounds like could be the cause. So I had to use 32 MinGW 5.3 for a reason; MinGW 7.3 supports 64 bit but as libbiosig doesn't build on Windows (I only have a pre-built 32 bit lib) I had to use MinGW 5.3 in this case.

I would argue that this change is necessary to avoid unexpected results - and to have backward compatibility.

Talking about code simplicity and beautification - I know this library is quite messy, there is a lot of things that can be optimized. I will try to do that in the following days, but of course, in separate PRs.

[cbrnr]

MinGW 7.3 supports 64 bit but as libbiosig doesn't build on Windows (I only have a pre-built 32 bit lib) I had to use MinGW 5.3 in this case.

I keep forgetting this, thanks for the reminder. Do you think we can use the prebuilt Win64 binary libbiosig? http://biosig.sourceforge.net/download.html

[Yida-Lin]

Sounds good Clemens, I will give 64 bit libbiosig a try tomorrow.

[tstenner]

I have a modified biosig (+CMake) folder for biosig on Windows somewhere to test if it's possible to build it with MSVC, but the C support for MSVC is atrocious. Building on Windows with MinGW requires some minor modifications, but it's possible:

diff --git a/biosig4c++/CMakeLists.txt b/biosig4c++/CMakeLists.txt
index 08df8840..776c1675 100644
--- a/biosig4c++/CMakeLists.txt
+++ b/biosig4c++/CMakeLists.txt
@@ -1,34 +1,11 @@
-cmake_minimum_required (VERSION 2.6)
-project (libbiosig)
+cmake_minimum_required (VERSION 3.12)
+project (libbiosig
+    VERSION 2.0.2
+    LANGUAGES C
+    )
 
-option (USE_ZLIB "Add support for compressed files." OFF)
 option (USE_SUITESPARSE "Add support for automatic re-referencing." OFF)
 
-# Version number
-set (libbiosig_VERSION_MAJOR 1)
-set (libbiosig_VERSION_MINOR 8)
-set (libbiosig_VERSION_PATCH 4)
-
-find_program (GAWK NAMES gawk)
-
-add_custom_target (eventcodes
-  COMMAND ${GAWK} -f eventcodes.awk extern/eventcodes.txt
-  WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
-  DEPENDS eventcodes.awk extern/eventcodes.txt
-)
-
-add_custom_target (units
-  COMMAND ${GAWK} -f units.awk extern/units.csv > "units.i"
-  WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
-  DEPENDS units.awk extern/units.csv
-)
-
-add_custom_target (annexb
-  COMMAND ${GAWK} -f annotatedECG.awk extern/11073-10102-AnnexB.txt > "11073-10102-AnnexB.i"
-  WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
-  DEPENDS annotatedECG.awk extern/11073-10102-AnnexB.txt
-)
-
 set (headers
   t210/abfheadr.h
   XMLParser/tinyxml.h
@@ -61,7 +38,7 @@ set (sources
   physicalunits.c
 )
 
-if (WIN32)
+if (WIN32 AND NOT MINGW)
   list (APPEND sources
     win32/getdelim.c
     win32/getline.c
@@ -84,62 +61,16 @@ add_library (biosigstatic STATIC
 )
 set_target_properties(biosigstatic PROPERTIES OUTPUT_NAME biosig)
 
-add_library (biosig2static STATIC
-  ${headers}
-  biosig2.h
-  biosig-dev.h
-  ${sources}
-  biosig2.c
-)
-set_target_properties(biosig2static PROPERTIES OUTPUT_NAME biosig2)
-
-add_library (biosigshared SHARED
-  ${headers}
-  ${sources}
-)
-set_target_properties(biosigshared PROPERTIES OUTPUT_NAME biosig)
-if (APPLE)
-  set_target_properties(biosigshared PROPERTIES
-     INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}/lib"
-     )
-endif ()
-
-add_library (biosig2shared SHARED
-  ${headers}
-  biosig2.h
-  biosig-dev.h
-  ${sources}
-  biosig2.c
-)
-set_target_properties(biosig2shared PROPERTIES OUTPUT_NAME biosig2)
 if (APPLE)
   set_target_properties(biosigshared PROPERTIES
      INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}/lib"
      )
 endif ()
 
-add_dependencies (biosigstatic eventcodes units annexb)
-add_dependencies (biosigshared eventcodes units annexb)
-add_dependencies (biosig2static eventcodes units annexb)
-add_dependencies (biosig2shared eventcodes units annexb)
-
-FIND_LIBRARY( ICONV_LIBRARY NAMES iconv )
-MARK_AS_ADVANCED( ICONV_LIBRARY )
-target_link_libraries( biosigstatic ${ICONV_LIBRARY} )
-target_link_libraries( biosigshared ${ICONV_LIBRARY} )
-target_link_libraries( biosig2static ${ICONV_LIBRARY} )
-target_link_libraries( biosig2shared ${ICONV_LIBRARY} )
-
-if (USE_ZLIB)
-  add_definitions (-D=WITH_ZLIB)
-  find_package( ZLIB REQUIRED )
-    if ( ZLIB_FOUND )
-      include_directories( ${ZLIB_INCLUDE_DIRS} )
-      target_link_libraries( biosigstatic ${ZLIB_LIBRARIES} )
-      target_link_libraries( biosigshared ${ZLIB_LIBRARIES} )
-      target_link_libraries( biosig2static ${ZLIB_LIBRARIES} )
-      target_link_libraries( biosig2shared ${ZLIB_LIBRARIES} )
-    endif( ZLIB_FOUND )
+find_package(ZLIB)
+if (ZLIB::ZLIB)
+	target_compile_definitions (biosigstatic PRIVATE WITH_ZLIB)
+	target_link_libraries( biosigstatic PUBLIC ZLIB::ZLIB )
 endif ()
 
 if (USE_SUITESPARSE)
@@ -147,18 +78,9 @@ if (USE_SUITESPARSE)
   FIND_LIBRARY( CHOLMOD_LIBRARY NAMES cholmod )
   MARK_AS_ADVANCED( CHOLMOD_LIBRARY )
   target_link_libraries( biosigstatic ${CHOLMOD_LIBRARY} )
-  target_link_libraries( biosigshared ${CHOLMOD_LIBRARY} )
-  target_link_libraries( biosig2static ${CHOLMOD_LIBRARY} )
-  target_link_libraries( biosig2shared ${CHOLMOD_LIBRARY} )
 endif ()
 
 add_definitions (-D=WITHOUT_NETWORK)
 
 install (TARGETS biosigstatic DESTINATION lib)
-install (TARGETS biosigshared DESTINATION lib)
-install (TARGETS biosig2static DESTINATION lib)
-install (TARGETS biosig2shared DESTINATION lib)
 install (FILES  biosig.h DESTINATION include)
-install (FILES  biosig2.h DESTINATION include)
-install (FILES  biosig-dev.h DESTINATION include)
-install (FILES  gdftime.h DESTINATION include)
diff --git a/biosig4c++/biosig.c b/biosig4c++/biosig.c
index 5872cef3..179e3d3f 100644
--- a/biosig4c++/biosig.c
+++ b/biosig4c++/biosig.c
@@ -53,7 +53,9 @@
 #include <locale.h>
 #include <math.h>      // define macro isnan()
 #include <stdlib.h>
+#define sopen(...) sopen_noclash()
 #include <sys/stat.h>
+#undef sopen
 
 
 #ifdef WITH_CURL
diff --git a/biosig4c++/t210/sopen_axg_read.c b/biosig4c++/t210/sopen_axg_read.c
index 1c5d2c43..63bd8321 100644
--- a/biosig4c++/t210/sopen_axg_read.c
+++ b/biosig4c++/t210/sopen_axg_read.c
@@ -23,7 +23,9 @@
 #include <ctype.h>
 #include <stdlib.h>
 #include <string.h>
+#define sopen(...) sopen_noclash()
 #include <sys/stat.h>
+#undef sopen
 #include <iconv.h>
 
 #if !defined(__APPLE__) && defined (_LIBICONV_H)
diff --git a/biosig4c++/t210/sopen_heka_read.c b/biosig4c++/t210/sopen_heka_read.c
index d256a15d..0234c4bf 100644
--- a/biosig4c++/t210/sopen_heka_read.c
+++ b/biosig4c++/t210/sopen_heka_read.c
@@ -24,7 +24,9 @@
 #include <ctype.h>
 #include <stdlib.h>
 #include <string.h>
+#define sopen(...) sopen_noclash()
 #include <sys/stat.h>
+#undef sopen
 
 #include "../biosig.h"
 
diff --git a/biosig4c++/test0/sandbox.c b/biosig4c++/test0/sandbox.c
index eed47b7f..87cebe4c 100644
--- a/biosig4c++/test0/sandbox.c
+++ b/biosig4c++/test0/sandbox.c
@@ -26,7 +26,9 @@
 #include <ctype.h>
 #include <stdlib.h>
 #include <string.h>
+#define sopen(...) sopen_noclash()
 #include <sys/stat.h>
+#undef sopen
 
 #include "../biosig.h"
 

[Yida-Lin]

Seems having some trouble getting SigViewer to build using a 64 libbiosig

[Yida-Lin]

I have a modified biosig (+CMake) folder for biosig on Windows somewhere to test if it's possible to build it with MSVC, but the C support for MSVC is atrocious. Building on Windows with MinGW requires some minor modifications, but it's possible:

This seems like a git log instead of an actual CMake file.

We can discuss more on migrating to 64 bit, but IMO this PR is still necessary; it took me quite a while to trace the bug down, leaving it the way it is might cause some other developers losing time as well unnecessarily.

[tstenner]

but IMO this PR is still necessary

I believe that it changes something, but I don't see how it fixes the underlying bug.

I've added a simple xdf loader in #26 that will fail to load your files if it's really a libxdf bug.

[Yida-Lin]

Hi @cbrnr is this bug reproducible on MacOS?

[cbrnr]

Should I test this with the current master or this branch?

[Yida-Lin]

@cbrnr Yes that would be great

[cbrnr]

Nope, doesn't freeze on macOS, I can open XDF files.

[cbrnr]

So how do we proceed? I've lost track of the current status. Which bugs have been squashed and which bugs still remain to be solved?

[Yida-Lin]

Not too sure; seems only Win 32 platform is affected. For me it's probably easier to merge this one as later I don't have to manually make these changes when I debug something. Also if somebody (other than SigViewer, not sure whether there is any) on Win 32 uses this library, that will cause problems as well. But as we don't provide SigViewer Win 32 release, I am also OK if this is left not merged.

Btw @cbrnr this is just a single bug, which is only reproducible on Win 32 so far and causes the application to freeze when opening any XDF file. I think I earlier tried 64bit libbiosig but it did not compile.

[cbrnr]

Alright, if this PR doesn't have any negative side effects please go ahead and merge.

[Yida-Lin]

Alright, if this PR doesn't have any negative side effects please go ahead and merge.

Hi @tstenner would you be OK with this?

[tstenner]

It doesn't have any negative side effects (other than an additional commit in the git history), because it doesn't have any real effects. You can check this with
compiler explorer, where most compilers will remove the temporary variable so the generated assembly for both functions is identical.