Opened 2 years ago

Closed 13 months ago

Last modified 13 months ago

#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: master
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

     
    5353int
    5454CommonBits::getBit(int64 bits, int i)
    5555{
    56         int64 mask = (1 << i);
     56        int64 mask = (1ull << i);
    5757        return (bits & mask) != 0 ? 1 : 0;
    5858}

UBSan: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html

Change History (5)

comment:1 Changed 2 years ago by strk

Any chance you could also provide a testcase showing how the current implementation fails and the patch succeeds ?

comment:2 Changed 2 years ago by goatbar

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:

Error: Failed to load processor go
No macro or processor named 'go' found

With that patch applied, all tests pass.

Last edited 2 years ago by goatbar (previous) (diff)

comment:4 Changed 13 months ago by dbaston

Resolution: fixed
Status: newclosed

Resolved in d390b76/git

comment:5 Changed 13 months ago by robe

Milestone: 3.7.0
Note: See TracTickets for help on using tickets.