John Naylor <johncnaylorls@gmail.com> writes:
> Done. I pushed this with a few last-minute cosmetic adjustments. This
> has been a very long time coming, but we're finally in the home
> stretch!
I'm not sure why it took a couple weeks for Coverity to notice
ee1b30f12, but it saw it today, and it's not happy:
/srv/coverity/git/pgsql-git/postgresql/src/include/lib/radixtree.h: 1621 in local_ts_extend_down()
1615 node = child;
1616 shift -= RT_SPAN;
1617 }
1618
1619 /* Reserve slot for the value. */
1620 n4 = (RT_NODE_4 *) node.local;
>>> CID 1594658: Integer handling issues (BAD_SHIFT)
>>> In expression "key >> shift", shifting by a negative amount has undefined behavior. The shift amount, "shift",
isas little as -7.
1621 n4->chunks[0] = RT_GET_KEY_CHUNK(key, shift);
1622 n4->base.count = 1;
1623
1624 return &n4->children[0];
1625 }
1626
I think the point here is that if you start with an arbitrary
non-negative shift value, the preceding loop may in fact decrement it
down to something less than zero before exiting, in which case we
would indeed have trouble. I suspect that the code is making
undocumented assumptions about the possible initial values of shift.
Maybe some Asserts would be good? Also, if we're effectively assuming
that shift must be exactly zero here, why not let the compiler
hard-code that?
- n4->chunks[0] = RT_GET_KEY_CHUNK(key, shift);
+ n4->chunks[0] = RT_GET_KEY_CHUNK(key, 0);
regards, tom lane