hi,
this fixes coverity bug #845, if group is NULL,
we dereference it when setting up dentry.
Signed-off-by: Eric Sesterhenn <[email protected]>
--- linux-2.6.16/fs/configfs/dir.c.orig 2006-03-23 00:02:23.000000000 +0100
+++ linux-2.6.16/fs/configfs/dir.c 2006-03-23 00:03:49.000000000 +0100
@@ -500,7 +500,7 @@ static int create_default_group(struct c
static int populate_groups(struct config_group *group)
{
struct config_group *new_group;
- struct dentry *dentry = group->cg_item.ci_dentry;
+ struct dentry *dentry;
int ret = 0;
int i;
@@ -512,6 +512,8 @@ static int populate_groups(struct config
* parent to find us, let alone mess with our tree.
* That said, taking our i_mutex is closer to mkdir
* emulation, and shouldn't hurt. */
+ dentry = group->cg_item.ci_dentry;
+
mutex_lock(&dentry->d_inode->i_mutex);
for (i = 0; group->default_groups[i]; i++) {
On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> this fixes coverity bug #845, if group is NULL,
> we dereference it when setting up dentry.
Is the converity checker merly looking at in-function patterns?
Where can I access the bug report (sorry for the question).
group cannot be null here, we aren't called any other way. So
while you are correct that the code below is needed in the presence of a
NULL group, really the "if (group" isn't necessary, just the "if
(group->default_groups)". I could even BUG_ON() if you'd like.
Joel
>
> Signed-off-by: Eric Sesterhenn <[email protected]>
>
> --- linux-2.6.16/fs/configfs/dir.c.orig 2006-03-23 00:02:23.000000000 +0100
> +++ linux-2.6.16/fs/configfs/dir.c 2006-03-23 00:03:49.000000000 +0100
> @@ -500,7 +500,7 @@ static int create_default_group(struct c
> static int populate_groups(struct config_group *group)
> {
> struct config_group *new_group;
> - struct dentry *dentry = group->cg_item.ci_dentry;
> + struct dentry *dentry;
> int ret = 0;
> int i;
>
> @@ -512,6 +512,8 @@ static int populate_groups(struct config
> * parent to find us, let alone mess with our tree.
> * That said, taking our i_mutex is closer to mkdir
> * emulation, and shouldn't hurt. */
> + dentry = group->cg_item.ci_dentry;
> +
> mutex_lock(&dentry->d_inode->i_mutex);
>
> for (i = 0; group->default_groups[i]; i++) {
>
>
--
"Win95 file and print sharing are for relatively friendly nets."
- Paul Leach, Microsoft
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
hi,
On Wed, 2006-03-22 at 15:27 -0800, Joel Becker wrote:
> On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> > this fixes coverity bug #845, if group is NULL,
> > we dereference it when setting up dentry.
>
> Is the converity checker merly looking at in-function patterns?
afaik it also looks what the functions which get called do. If you call
a function that might free a pointer you pass, it warns if you use
it afterwards.
> Where can I access the bug report (sorry for the question).
I would guess [email protected]
> group cannot be null here, we aren't called any other way. So
> while you are correct that the code below is needed in the presence of a
> NULL group, really the "if (group" isn't necessary, just the "if
> (group->default_groups)". I could even BUG_ON() if you'd like.
I would then propose the following patch, so the check can be
removed for people who like small kernels. I dont think gcc notices
that all callers use non-NULL values and optimizes it away.
--- linux-2.6.16/fs/configfs/dir.c.orig 2006-03-23 00:31:16.000000000 +0100
+++ linux-2.6.16/fs/configfs/dir.c 2006-03-23 00:32:07.000000000 +0100
@@ -504,7 +504,9 @@ static int populate_groups(struct config
int ret = 0;
int i;
- if (group && group->default_groups) {
+ BUG_ON(!group); /* group == NULL is not allowed */
+
+ if (group->default_groups) {
/* FYI, we're faking mkdir here
* I'm not sure we need this semaphore, as we're called
* from our parent's mkdir. That holds our parent's
On Thu, Mar 23, 2006 at 12:36:54AM +0100, Eric Sesterhenn wrote:
> I would then propose the following patch, so the check can be
> removed for people who like small kernels. I dont think gcc notices
> that all callers use non-NULL values and optimizes it away.
>
> - if (group && group->default_groups) {
> + BUG_ON(!group); /* group == NULL is not allowed */
> +
> + if (group->default_groups) {
This is pretty much what I meant. Thanks.
Joel
--
Life's Little Instruction Book #3
"Watch a sunrise at least once a year."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Wed, Mar 22, 2006 at 03:27:09PM -0800, Joel Becker wrote:
> On Thu, Mar 23, 2006 at 12:05:29AM +0100, Eric Sesterhenn wrote:
> > this fixes coverity bug #845, if group is NULL,
> > we dereference it when setting up dentry.
>
> Is the converity checker merly looking at in-function patterns?
> Where can I access the bug report (sorry for the question).
> group cannot be null here, we aren't called any other way. So
> while you are correct that the code below is needed in the presence of a
> NULL group, really the "if (group" isn't necessary, just the "if
> (group->default_groups)". I could even BUG_ON() if you'd like.
Coverity is correct that something is wrong:
<-- snip -->
...
struct dentry *dentry = group->cg_item.ci_dentry;
int ret = 0;
int i;
if (group && group->default_groups) {
...
<-- snip -->
The question is only whether the dereferencing is a real bug or there's
only the harmless issue of a superfluous check.
> Joel
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Thu, Mar 23, 2006 at 12:36:54AM +0100, Eric Sesterhenn wrote:
>...
> On Wed, 2006-03-22 at 15:27 -0800, Joel Becker wrote:
>...
> > group cannot be null here, we aren't called any other way. So
> > while you are correct that the code below is needed in the presence of a
> > NULL group, really the "if (group" isn't necessary, just the "if
> > (group->default_groups)". I could even BUG_ON() if you'd like.
>
> I would then propose the following patch, so the check can be
> removed for people who like small kernels. I dont think gcc notices
> that all callers use non-NULL values and optimizes it away.
>
> --- linux-2.6.16/fs/configfs/dir.c.orig 2006-03-23 00:31:16.000000000 +0100
> +++ linux-2.6.16/fs/configfs/dir.c 2006-03-23 00:32:07.000000000 +0100
> @@ -504,7 +504,9 @@ static int populate_groups(struct config
> int ret = 0;
> int i;
>
> - if (group && group->default_groups) {
> + BUG_ON(!group); /* group == NULL is not allowed */
> +
> + if (group->default_groups) {
>...
Why do we need a BUG_ON() if we already got an Oops?
Simply remove the check.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed