lkml.org 
[lkml]   [2000]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: an assortment of memory alloc/dealloc errors
Date
Hi David,

my apologies for the spurious messages. I'd thought, but did not type,
the warning that the messages after the annotated errors had not been
inspected yet. They were just included for the curious. It looks like
the networking code is more exhuberant with pointers and such, so you
got a much higher number of false positives than the other code we've
looked at.


> From: Dawson Engler <engler@csl.Stanford.EDU>
> Date: Tue, 8 Aug 2000 18:40:17 -0700 (PDT)
>
> /u2/engler/ic/linux-2.3.99/net/core/dev_mcast.c:174:dev_mc_add: ERROR: Unchecked use of malloc'd var 'dmi1'
>
> What is wrong here? We check it for NULL (via the assignment to
> 'dmi', maybe this is tripping up your g++ extension) and drop a lock
> and return an error code if so.

It's just a checker mis-feature: it does not propogate assignments, so
misses the fact that this:

if ((dmi = dmi1) == NULL) {
write_unlock_bh(&dev_mc_lock);
return -ENOMEM;
}

checks dmi1 against null. It's not hard to add, but isn't in there.

>
> /u2/engler/ic/linux-2.3.99/net/core/scm.c:94:scm_fp_copy: ERROR: did not free fpl on error path
> /u2/engler/ic/linux-2.3.99/net/core/scm.c:82:scm_fp_copy: ERROR: did not free fpl on error path
>
> The data behind 'fpl' is given to the caller by the assignment to
> *fplp, it cannot be freed. I believe the code is correct.
>
> I honestly think that you need to teach your checker that if the
> pointer is stored into memory before function return, it is in fact
> correct to not free the memory even for an error path.

It'd be a good option to have, but it's not quite so simple. Two
exceptions:
1. You probably don't want to do this if you stored the pointer
in memory only pointed to by a local pointer, since this root
will be lost when you return.

2. In many places, even though you do store the pointer in
persistent memory, the retry code does not check if this
pointer is non-nil and allocates over it again.

Another possibe addition is to not flag cases where the allocated
storage was passed to another function.

> (in fact, if it did [free somethig pointed to by memory], that is a
> different bug which should be pointed out :-)

This sounds like a nice check --- thanks!

> /u2/engler/ic/linux-2.3.99/net/sched/cls_tcindex.c:293:tcindex_change: ERROR: did not free p on error path
> /u2/engler/ic/linux-2.3.99/net/sched/cls_tcindex.c:293:tcindex_change: ERROR: did not free p on error path
>
> I can't discern what your checker is trying to point out here.

Unfortunately, the error message stripped off the field names it was worried
about. It's warning that 'p->perfect' and 'p->h' could be lost

}
if (!p->perfect && !p->h) {
p->alloc_hash = p->hash;
DPRINTK("hash %d mask %d\n",p->hash,p->mask);
if (p->hash > p->mask) {
p->perfect = kmalloc(p->hash*
sizeof(struct tcindex_filter_result),GFP_KERNEL);
if (!p->perfect)
return -ENOMEM;
memset(p->perfect, 0,
p->hash * sizeof(struct tcindex_filter_result));
} else {
p->h = kmalloc(p->hash*sizeof(struct tcindex_filter *),
GFP_KERNEL);
if (!p->h)
return -ENOMEM;
memset(p->h, 0, p->hash*sizeof(struct tcindex_filter *));
} }
if (handle > p->mask)
return -EINVAL;

Assuming the code is deterministic, it looks like this is a false positive.

> Yep broken, I've made a note to fix this file because it will take
> a bit more than simple error return addition to fix the problem
> properly.

It's good to see it wasn't a complete waste of time...

> /u2/engler/ic/linux-2.3.99/net/ipv4/ip_options.c:513:ip_options_get: ERROR: did not free opt on error path
>
> It did free it in this case.

Indeed. This run of the checker was not aware that kfree_s was a
freeing function (it was just looking for kfree).


> /u2/engler/ic/linux-2.3.99/net/netlink/af_netlink.c:799:netlink_dump_start: ERROR: did not free cb on error path
> /u2/engler/ic/linux-2.3.99/net/netlink/af_netlink.c:791:netlink_dump_start: ERROR: did not free cb on error path
>
> Your checker obviously doesn't understand that
> netlink_destroy_callback frees up the memory because your checker does
> not do any global analysis :-)))

Yeah. Well, you get what you pay for. ;-) A student, Yu Ping, is hacking
on global analysis as we speak.


> /u2/engler/ic/linux-2.3.99/net/decnet/dn_fib.c:364:dn_fib_create_info: ERROR: Using 'fi' illegally!
>
> Same as ipv4/fib_semantics.c above, I have no idea what your checker
> is trying to say here.

This is a checker mis-freature. It looks like the state ``is a null
pointer'' does not have a rule allowing you to compare the pointer to
null again. Easy to fix.

Apologies again for not warning that the messages had not been
examined. I have to say, your code ran into a bunch of false positive
cases I hadn't seen yet (and this is after going over 200+ messages in
Linux, OpenBSD, and the Xok exokernel ;-).

Dawson

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:58    [W:0.303 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site