On 10/31/11 00:43, NeilBrown wrote:
> On Fri, 28 Oct 2011 21:50:50 +0200 Jes.Sorensen@xxxxxxxxxx wrote:
>
>> > From: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>> >
>> > If picking just one spare disk from the container, jump out of the
>> > loop once freeing the list. Otherwise we end up accessing the list
>> > that we just freed.
>> >
>> > Signed-off-by: Jes Sorensen <Jes.Sorensen@xxxxxxxxxx>
>> > ---
>> > util.c | 2 ++
>> > 1 files changed, 2 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/util.c b/util.c
>> > index 2cf617d..1bbd87f 100644
>> > --- a/util.c
>> > +++ b/util.c
>> > @@ -1766,6 +1766,7 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>> > if (get_one) {
>> > sysfs_free(*dp);
>> > d->next = NULL;
>> > + goto out;
>> > }
>> > } else {
>> > *dp = d->next;
>> > @@ -1773,5 +1774,6 @@ struct mdinfo *container_choose_spares(struct supertype *st,
>> > sysfs_free(d);
>> > }
>> > }
>> > +out:
>> > return disks;
>> > }
>
> Hi Jes,
> I dont' think patch is needed.
> The while loop that it jumps out of is
> while (*dp)
>
> at the place you put the goto,
> dp == &d->next
> As d->next was just set to NULL, *dp will be NULL, so the loop will
> exit with the need for a goto.
Hi Neil,
It has to be said, I didn't actually come up with this one on my own, it
was found by the Coverity security checker. I stared at this one for
quite a while. It is well obfuscated. So far I managed to convince
myself several times that you were right and also that Coverity is
correct. In the end I think that you are right indeed and this is a
false positive.
However, in order to avoid the confusion, how about changing the d->next
= NULL assignment to *dp = NULL instead? It should make it a lot more
clear for anyone reading the code what is going on.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
