[geos-devel] [GEOS] #869: CommonBits::zeroLowerBits undefined behavior

[GEOS]

#869: CommonBits::zeroLowerBits undefined behavior
-------------------------------+---------------------------
 Reporter:  goatbar            |       Owner:  geos-devel@…
     Type:  defect             |      Status:  reopened
 Priority:  minor              |   Milestone:  3.6.4
Component:  Default            |     Version:  3.6.2
 Severity:  Unassigned         |  Resolution:
 Keywords:  UndefinedBehavior  |
-------------------------------+---------------------------
Changes (by rouault):

 * status:  closed => reopened
 * resolution:  fixed =>


Comment:

 Actually, the fix is incorrect for several reasons:
 - (1<< nBits)-1  has only int operands, so will be evaluated on 32 bit
 only. Consequently nBits >= 32 will result in incorrect behaviour, unless
 you cast the first 1 to int64 :  ((int64)1 << nBits) - 1
 - but, if let to 32 bits ( (1<< nBits)-1 ), then nBits == 31 is also an
 invalid input (-fsanitize=undefined warns about "runtime error: left shift
 of 1 by 31 places cannot be represented in type 'int'") even if "sane"
 compilers / most CPUs will finally do the right thing. The right fix in
 that case to avoid undefined/implementation defined behaviour is to use a
 unsigned 1, so (1U << nBits) - 1
 - for the same reason, is the mask is intended to be a 64 bit one, so
 ((int64)1 << nBits) - 1), then nBits == 63 also triggers undefined
 behaviour. And thus the right fix would be ((unsigned int64)1 << nBits) -
 1

-- 
Ticket URL: <https://trac.osgeo.org/geos/ticket/869#comment:5>
GEOS <http://trac.osgeo.org/geos>
GEOS (Geometry Engine - Open Source) is a C++ port of the Java Topology Suite (JTS).