PATCH: ctdb: buffer write beyond limits

[Christof Schmitt]

On Tue, Feb 19, 2019 at 12:37:18PM +0100, swen via samba-technical wrote:
> On Tue, 2019-02-19 at 11:53 +0100, Volker Lendecke wrote:
> > On Tue, Feb 19, 2019 at 11:02:18AM +0100, Volker Lendecke via samba-
> > technical wrote:
> > > On Tue, Feb 19, 2019 at 10:53:41AM +0100, swen wrote:
> > > > the code is right if the following facts are taken into account.
> > > > 1. MAX_SIGNED_INT = MAX_UNSIGNED_INT/2 - 1
> > > > 2. length is defined as unsigned int and will get assigned at
> > > > most MAX_SIGNED_INT(num_ready)
> > > > 3. offset is initially zero and will only grow as much as length
> > > > is shrinking (=>code)
> > > > 	=> therefore, offset + length are at most MAX_SIGNED_INT.
> > > > Guaranteed !
> > > > 4. If then another (guaranteed positive) signed integer (here
> > > > num_ready) is added we simply
> > > >    cannot overflow an unsigned int because 
> > > >    MAX_SIGNED_INT + MAX_SIGNED_INT = MAX_UNSIGNED_INT - 2
> > > > 
> > > > Taken those simple maths into account, I hope you can agree to
> > > > the code.
> > > 
> > > No, I do not agree. There is a simple boiler plate to do a checked
> > > addition, and a simple boiler plate to do checked subtraction.
> > > Please
> > > do it that way.
> > 
> > More explanation: I do not agree because it is really simple to get
> > these lines right on their own respect. And having to read a 4-entry
> > proof referencing code elsewhere where it's easy to make to code safe
> > in an isolated manner.
> > 
> > The crash was introduced through patches that did not affect this MIN
> > statement at all. This means that the environment can change, and the
> > assumptions that you refer to in bullet point 3 might change, causing
> > the code to crash again. In this kind of code we have to check every
> > individual arithmetic operation in isolation.
> > 
> > Attached find something that I did in 10 minutes. Please crush it if
> > you think your approach is better.

Considering the complexity here: Would it be possible to add unit tests
here? First to ensure that the fix addresses the problem at hand and
then also to stress other corner cases. I am not yet sure what the best
strategy here would be. It could be unit testing the function (cmocka?) or
setting up a test client that sends the combinations of requests to ctdb
that we want to test. Either way, we should ensure that this works and
does not regress with future patches.

Christof