r-lidar / rlas

R package to read and write las and laz files used to store LiDAR data
https://cran.r-project.org/package=rlas
GNU General Public License v3.0
34 stars 14 forks source link

Additionnal issues with sanitizer #18

Closed Jean-Romain closed 6 years ago

Jean-Romain commented 6 years ago

@floriandeboissieu

I released rlas. Every regular tests are good but I received the following email from Prof. Bryan Ripley

See https://cran.r-project.org/web/checks/check_results_rlas.html (which is still updating).

This version has introduced serious 'Additional issues'. Please correct ASAP once all the results have been updated, and before Apr 23 to safely retain the package on CRAN.

Looking in the logs, it is an error with the memory sanitizer. ASAN and USBAN are diffcult tools to use. I'm definitively not able to reproduce and rhub::check_with_sanitizers is ok. Thus it is impossible to reproduce.

The good new is that the error on CRAN is informative:

writeLAS.cpp:383:48: runtime error: nan is outside the range of representable values of type 'int' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior writeLAS.cpp:383:48 in

So in my opinion it is related to no_data. I'm not fully comfortable with this part. You are the main author of this code. Could you please check that.

The support of NA is maybe not entirely correct.

Jean-Romain commented 6 years ago

And we have another one with GCC

 ==48816==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300005add2 at pc 0x7f6c33cba36e bp 0x7ffed8038b90 sp 0x7ffed8038338
  READ of size 19 at 0x60300005add2 thread T0
      #0 0x7f6c33cba36d  (/lib64/libasan.so.4+0x5136d)
      #1 0x7f6c301ea457 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&) (/lib64/libstdc++.so.6+0x128457)
      #2 0x7f6c21c14356 in vlrsreader(LASheader*) /data/gannet/ripley/R/packages/tests-gcc-SAN/rlas/src/readheader.cpp:215
floriandeboissieu commented 6 years ago

In my opinion:

Cheers

Florian

Jean-Romain commented 6 years ago

This commit make the code much more readable and ensure that NA will never be written in the file. I hope it fixes clang-ASAN and clang-USBAN errors.

gcc-ASAN still don't like std::string(reinterpret_cast< char const* >(U8*))

Jean-Romain commented 6 years ago

This commit disable some features. I'm not able to convert U8 into string without using `reinterpret_cast< char const >`. If these lines are really responsible of the ASAN error I'm not able to fix I remove the support for now.