On Mon, Mar 25, 2024 at 1:53 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > John Naylor <johncnaylo...@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!
Thank you for the report. > > 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: Hmm, I've also done Coverity Scan in development but I wasn't able to see this one for some reason... > > /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", is as 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); Sounds like a good solution. I've attached the patch for that. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
fix_radixtree.patch
Description: Binary data
