hi Dimitri, Zeroing the padding is probably a good idea; I'd be interested to look at the diff to see how many code paths are impacted. We already have a number of places where we are zeroing buffers but as you have found it is not 100% consistent.
- Wes On Tue, Jul 3, 2018 at 10:13 AM, Dimitri Vorona <[email protected]> wrote: > Hi, > > I misunderstood the issue: the zeroing of the padding is correctly handled > by the writers, as long as they are careful not to read beyond the > specified size of the buffers (as opposed to its capacity). > > The valgrind warning came from the way AppendNull(s) is implemented in some > of the builders: it just sets the nullmap and increments lengths, but > doesn't set the data, which leads to uninitialized memory chunks, where > nulls where inserted. I'll send a PR which addresses it. > > I'm still not sure if we should add zero the padding anyway, since the > buffers can be shared in any number of ways. Zeroing the last couple of > bytes shouldn't have any performance impact and could save a headache in > the future. > > Cheers. > > On Tue, Jul 3, 2018 at 1:15 PM Antoine Pitrou <[email protected]> wrote: > >> >> Hi, >> >> Writing uninitialized memory risks leaking private data. This might >> lead to security issues. >> >> I'd go for option 2. Option 3 sounds much more costly (we would be >> zero-initializing large memory areas instead of small padding areas). >> >> Regards >> >> Antoine. >> >> >> Le 03/07/2018 à 13:11, Dimitri Vorona a écrit : >> > Hi all, >> > >> > currently, running json-integration-test with valgrind leads to the >> > following warning: "Syscall param write(buf) points to uninitialised >> > byte(s)". This is caused by PrimitiveBufferBuilder not initializing its >> > data memory. Note: we initialize null_bitmap_data_ by zeroing, i.e. >> setting >> > all values to not-null by default. >> > >> > Since having tests pass valgrind might be desirable for the CI, I think >> we >> > should fix this warning. There are a couple of possibilities: >> > >> > 1. Add suppression. The specs doesn't require padding to have a specific >> > value, so we might consider it to be false positive >> > 2. Add initialization of the padding bytes to ArrayBuilder::FinishIntenal >> > implementations. >> > 3. Generally zero-initialize memory in PoolBuffer. Might be too >> expensive. >> > >> > Or course there could be number of other options, includeing "do >> nothing". >> > If we settle on a best option, I can make a PR. >> > >> > Cheers, >> > Dimitri. >> > >>
