gtkwave icon indicating copy to clipboard operation
gtkwave copied to clipboard

libfst have undefined behaviors catched by `-fsanitize=undefined`

Open FanShupei opened this issue 1 year ago • 8 comments

I catch several undefined behaviors with -fsantinize=underfined using verilator fst tracing. Since gtkwave is the upstream, I just report it here.

The first is related to FST_DO_MISALIGNED_OPS The second is an overflow bug in lz4.c

  1. For the first bug, I propose to completely remove FST_DO_MISALIGNED_OPS, for two reasons
  • misaligned pointer dereference is undefined behavior. see https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
  • misaligned pointer dereference provides no benefit than memcpy. In modern gcc/clang 'memcpy' is implemented as intrinsics. They generate nearly identical code for misaligned read/write, even in -O0. see https://godbolt.org/z/q5fr3Ksdr
  1. For the second bug, the fix is actually small. However I notice that lz4.c is pretty outdated. Any plan to bump its version from upstream?
diff --git a/lib/libfst/lz4.c b/lib/libfst/lz4.c
index 55dc1a8..893d6dc 100644
--- a/lib/libfst/lz4.c
+++ b/lib/libfst/lz4.c
@@ -536,7 +536,7 @@ FORCE_INLINE int LZ4_compress_generic(
         }
 
         /* Catch up */
-        while ((ip>anchor) && (match+refDelta > lowLimit) && (unlikely(ip[-1]==match[refDelta-1]))) { ip--; match--; }
+        while ((ip>anchor) && (match+refDelta > lowLimit) && (unlikely(ip[-1]==(match+refDelta)[-1]))) { ip--; match--; }
 
         {
             /* Encode Literal length */

I'm willing to authoring a pull request. Any comments?

FanShupei avatar Jan 13 '24 09:01 FanShupei