Given the discussion here, I tried making the buffer descriptor accessors take a signed index and adding range assertions.
Most callers already pass signed buffer indexes derived from Buffer values (e.g. buffer - 1, or -buffer - 1 for local buffers). The only caller I found that passes an unsigned value to GetBufferDescriptor() is ClockSweepTick(), and that normalizes the value before returning it, so it should still be less than NBuffers.
The change looked fairly straightforward to me, unless I'm missing something. I also made the same change for GetLocalBufferDescriptor(), since it has the same uint32 signature and takes local buffer indexes.
cfbot flagged v1 in an assertion-enabled (cassert) build: anything that uses a temporary table hits the new assertion in GetLocalBufferDescriptor():
It looks like InitLocalBuffers() initializes the descriptors in a loop before it sets NLocBuffer, so during that loop the assert sees NLocBuffer == 0. The access itself seems fine, since the array is already allocated with num_temp_buffers entries; it's just that this is the one place that runs before the bound the assert checks against is set. (The shared path doesn't run into this, since NBuffers is already set when BufferManagerShmemInit() does the equivalent loop.)
I'm not sure which way would be preferable here if are adding assert for
LocalBufferDescriptor too:
1. set NLocBuffer = nbufs before the init loop, so the loop can keep using GetLocalBufferDescriptor(); or 2. leave NLocBuffer where it is and index LocalBufferDescriptors[] directly in that loop.
