PATCH: ctdb: buffer write beyond limits

[swen]

On Tue, 2019-02-19 at 09:48 -0700, Christof Schmitt via samba-technical 
wrote:
> 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.

What made me believe that this could be an easy fix for an obvious issue ?

The code, as it is now, is wrong. This fix is a must !
The additional checks/verifications around the main fixing-code
are negotiable (to me) but I was hoping to finally have
the comprehensive patch at hand now.

Sorry Christof but where is the complexity and 
which corner cases could you think of ?

Adding tests is never a bad idea but considering where we are in the release phase
could we agree on a fix for the base-/understood-issue first ?
I'm getting the impression that the blanket solution might take a few more iterations.

Cheers Swen


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190219/0c71eb4b/signature.sig>