#834 closed defect (fixed)
UBSan fix for getBit in precision/CommonBits.cpp
Reported by: | goatbar | Owned by: | strk |
---|---|---|---|
Priority: | minor | Milestone: | 3.7.0 |
Component: | Core | Version: | main |
Severity: | Unassigned | Keywords: | ubsan |
Cc: |
Description
This is small enough that it doesn't seem to merit a pull request.
Found by and patch by Google engineer Han Shen:
The origin implementation never works for i >= 32, because literal "1" is 32-bit, thus "1<<i" is always 32-bit, "mask"'s higher 32-bit is always 0. Worse, "1 << i" causes undefined exception ("i" ranges from [1, 52]). Fixed by using "1ull" (64-bit) in place of "1".
-fsanitize=shift-exponent
-
src/precision/CommonBits.cpp
53 53 int 54 54 CommonBits::getBit(int64 bits, int i) 55 55 { 56 int64 mask = (1 << i);56 int64 mask = (1ull << i); 57 57 return (bits & mask) != 0 ? 1 : 0; 58 58 }
UBSan: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Change History (5)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
My env currently has geos 3.5.0, but this should be the same with 3.6.1 or head.
It was found with https://github.com/paulsmith/gogeos
Build with:
-fsanitize=shift-exponent -fsanitize-trap=shift-exponent
==================== Test output for //third_party/golang/geos/geos:geos_test: === RUN TestCoordSeq --- PASS: TestCoordSeq (0.00s) === RUN TestGeometryTypeMethod --- PASS: TestGeometryTypeMethod (0.00s) === RUN TestGeometryTypeString --- PASS: TestGeometryTypeString (0.00s) === RUN TestGeometryType --- PASS: TestGeometryType (0.00s) === RUN TestGeometryProject --- PASS: TestGeometryProject (0.00s) === RUN TestGeometryInterpolate --- PASS: TestGeometryInterpolate (0.00s) === RUN TestGeometryBuffer --- PASS: TestGeometryBuffer (0.00s) === RUN TestGeometryBufferWithOpts --- PASS: TestGeometryBufferWithOpts (0.00s) === RUN TestOffsetCurve SIGILL: illegal instruction PC=0xa3388a m=4 sigcode=2 signal arrived during cgo execution goroutine 40 [syscall, locked to thread]: runtime.cgocall(0x7b7e00, 0xc42003c660, 0x7f1aefa3b300) cgocall.go:131 +0xe2 fp=0xc42003c610 sp=0xc42003c5d0 pc=0x63f2d2 geos/geos._Cfunc_GEOSOffsetCurve_r(0x7f1aefa02420, 0x7f1aefa3b300, 0x3ff0000000000000, 0x100000008, 0x4014000000000000, 0x0) geos/_objs/geos_test_testlib/_cgo_gotypes.go:1370 +0x4e fp=0xc42003c660 sp=0xc42003c610 pc=0x73a8ce geos/geos.cGEOSOffsetCurve.func1(0x7f1aefa02420, 0x7f1aefa3b300, 0x3ff0000000000000, 0x100000008, 0x4014000000000000, 0x7f1aefa92cf0) geos/cwrappers.go:259 +0xcb fp=0xc42003c6a0 sp=0xc42003c660 pc=0x746c9b geos/geos.cGEOSOffsetCurve(0x7f1aefa3b300, 0x3ff0000000000000, 0x100000008, 0x4014000000000000, 0x0) geos/cwrappers.go:259 +0xa4 fp=0xc42003c6e0 sp=0xc42003c6a0 pc=0x73d7c4 geos/geos.(*Geometry).OffsetCurve(0xc42011e0d8, 0x3ff0000000000000, 0x8, 0x0, 0x1, 0x4014000000000000, 0x3b, 0xc42003c778, 0x698673) geos/geom.go:327 +0x5c fp=0xc42003c720 sp=0xc42003c6e0 pc=0x741f5c geos/geos.TestOffsetCurve(0xc4201464e0) geos/geom_test.go:110 +0xa4 fp=0xc42003c7a8 sp=0xc42003c720 pc=0x72f314
https://github.com/paulsmith/gogeos/blob/master/geos/geom_test.go#L107
From github:
With that patch applied, all tests pass.
comment:5 by , 6 years ago
Milestone: | → 3.7.0 |
---|
Any chance you could also provide a testcase showing how the current implementation fails and the patch succeeds ?