pcp
[Top] [All Lists]

Re: [pcp] memory corruption bug fix, libpcp p_instance.c

To: pcp@xxxxxxxxxxx
Subject: Re: [pcp] memory corruption bug fix, libpcp p_instance.c
From: Ken McDonell <kenj@xxxxxxxxxxxxxxxx>
Date: Fri, 06 Mar 2015 06:43:40 +1100
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <20150302235142.GG21203@xxxxxxxxxx>
References: <20150302235142.GG21203@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
On 03/03/15 10:51, Frank Ch. Eigler wrote:
>
The following one-liner reproduces the problem, but I couldn't
pick a favorite qa/NNN to plop that into.  Any nominations from
the Academy?

valgrind pmval 'pmcd.buf.alloc' -i \
   `awk 'BEGIN { for (i=0;i<3000;i++) { printf "x" }; printf("\n"); }'`

This is a classical example of where a unit test should _not_ be added to an existing test, rather a new test should be created. If we added it to an existing test that would fail until the code fix is applied which would cause the whole test to be seen as failing. That is confusing and months, years, decades later when it regresses and fails for some reason the triage is so much more complicated.

Making a new test is so simple ...

$ cd qa
$ ./new
creates a new skeletal test and places you in your ${EDITOR-vi} of choice
add your 2 or 3 line shell script into the right place ... look for the line
# real QA test starts here
$ ./remake
and when your done add and commit your spanking new NNN and NNN.out along with the "group" file (that new also updates)

I've done this for you case ... it is qa/874


commit 0629116e49c70c5e3d86570807c563158ccf576d
Author: Frank Ch. Eigler <fche@xxxxxxxxxx>
Date:   Mon Mar 2 18:32:20 2015 -0500

     libpcp memory corruption bug: __pmSendInstanceReq doing __pmFindPDU too 
small

     An exact-size __pmFindPDU malloc implementation found this bug in
     p_instance.c, wherein the `sizeof(need)' rather than `need' was
     passed.  This corrupted memory behind the declared region, but the
     problem was hidden because the clasical __pmFindPDU rounded up memory
     allocations to 1K+, but large indom-name queries can trigger it.

Good catch.

Cherrypicked, reviewed in my tree.

Thanks, Frank.

<Prev in Thread] Current Thread [Next in Thread>