hi Dimitri, On Mon, Jul 9, 2018 at 4:12 AM, Dimitri Vorona <[email protected]> wrote: > Hi all, > > now that PR 2216 has been merged, many of the mentioned problems have been > alleviated. To prevent similar problems, I'll add, as Philipp suggested, a > debug-build check to the buffer builders, which ensures that the finish > method has been called before the builder destructs.
Great, this sounds useful. The downside to this is that it would cause debug builds to terminate if the builder is destructed for some other reason. It might be better to just have logging output in debug builds rather than a DCHECK > > Unfortunately, not every buffer is built with the builders. Since the > buffers are allocated uninitialized, it's really easy to forget about > padding. The Copy method of the buffer, for example, leaves the padding of > the target buffer uninitialized, except in the rare case where size_ == > capacity_. Basically, before a newly allocated buffer is used, you have to > make sure it's zero padded. Thank you for catching this; I agree we should always zero the bytes between size_ and capacity_. We could go even further by putting this logic in the buffer implementations (Resize, etc.) > > I experimented with changing Buffer::data method's semantics to check if > the buffer is zero padded in the debug builds. It turned up a number of > problems (the ray one including), but also a great deal of false positives, > so I'm not sure if it's the right way forward. > > I think we should at least add a AllocateBufferZeroPadded method and make > explicit, that you should use it, if you don't plan on resizing the buffer > after allocation. Also, a Buffer::ZeroPadding method which memsets > everything between size_ and capacity_ to 0 might be useful, since it would > catch programmers attention to the need. Possible checks can then be added > as needed. Sounds good to me. Thank you for looking into this. > > Cheers, > Dimitri.
