Fix UDF-FS potentially dereferencing null

Fix UDF-FS potentially dereferencing null

[Dave Jones]

Move size instantiation after null check for 'dir', nearer
to where its first used.

		Dave

--- linux-2.6.5/fs/udf/namei.c~	2004-04-16 22:38:28.000000000 +0100
+++ linux-2.6.5/fs/udf/namei.c	2004-04-16 22:39:25.000000000 +0100
@@ -159,7 +159,7 @@
 	char *nameptr;
 	uint8_t lfi;
 	uint16_t liu;
-	loff_t size = (udf_ext0_offset(dir) + dir->i_size) >> 2;
+	loff_t size;
 	lb_addr bloc, eloc;
 	uint32_t extoffset, elen, offset;
 	struct buffer_head *bh = NULL;
@@ -202,6 +202,8 @@
 		return NULL;
 	}
 
+	size = (udf_ext0_offset(dir) + dir->i_size) >> 2;
+
 	while ( (f_pos < size) )
 	{
 		fi = udf_fileident_read(dir, &f_pos, fibh, cfi, &bloc, &extoffset, &eloc, &elen, &offset, &bh);

Re: Fix UDF-FS potentially dereferencing null

[Linus Torvalds]

Again, "dir" cannot be NULL here, that would be a much more fundamental 
bug and just impossible (the way we get to this thing is to follow the 
directory operations - which we find by looking at "dir").

Maybe we could tell the compiler that "dir" is safe to dereference some 
way? Or add a 'sparse' annotation about safe pointers?

I'd rather just remove the bogus check for a NULL dir pointer..

		Linus

On Fri, 16 Apr 2004, Dave Jones wrote:
>
> Move size instantiation after null check for 'dir', nearer
> to where its first used.
> 
> 		Dave
> 
> --- linux-2.6.5/fs/udf/namei.c~	2004-04-16 22:38:28.000000000 +0100
> +++ linux-2.6.5/fs/udf/namei.c	2004-04-16 22:39:25.000000000 +0100
> @@ -159,7 +159,7 @@
>  	char *nameptr;
>  	uint8_t lfi;
>  	uint16_t liu;
> -	loff_t size = (udf_ext0_offset(dir) + dir->i_size) >> 2;
> +	loff_t size;
>  	lb_addr bloc, eloc;
>  	uint32_t extoffset, elen, offset;
>  	struct buffer_head *bh = NULL;
> @@ -202,6 +202,8 @@
>  		return NULL;
>  	}
>  
> +	size = (udf_ext0_offset(dir) + dir->i_size) >> 2;
> +
>  	while ( (f_pos < size) )
>  	{
>  		fi = udf_fileident_read(dir, &f_pos, fibh, cfi, &bloc, &extoffset, &eloc, &elen, &offset, &bh);
> 

Re: Fix UDF-FS potentially dereferencing null

[viro]

On Fri, Apr 16, 2004 at 10:41:04PM +0100, Dave Jones wrote:
> Move size instantiation after null check for 'dir', nearer
> to where its first used.
 
Check in question is a BS - it never gets NULL passed as dir.

Re: Fix UDF-FS potentially dereferencing null

[Jeff Garzik]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Fri, Apr 16, 2004 at 10:41:04PM +0100, Dave Jones wrote:
> 
>>Move size instantiation after null check for 'dir', nearer
>>to where its first used.
> 
>  
> Check in question is a BS - it never gets NULL passed as dir.


Yes, it looks like a lot of these NULL checks caught can be fixed simply 
by removing bogus and/or redundant checks.

	Jeff

Re: Fix UDF-FS potentially dereferencing null

[Dave Jones]

On Fri, Apr 16, 2004 at 07:13:04PM -0400, Jeff Garzik wrote:
 > >Check in question is a BS - it never gets NULL passed as dir.
 > 
 > Yes, it looks like a lot of these NULL checks caught can be fixed simply 
 > by removing bogus and/or redundant checks.

And there's a *lot* of them. Those half dozen or so patches earlier were
results of just a quick random skim of the list the coverity folks came up with.

It'll take a lot of effort to 'fix' them all, and given the non-severity
of a lot of them, I'm not convinced it's worth the effort.

Some of them are obvious bugs (like the edd patches), but others are
a little less obvious, and thats where a lot of time can be wasted.

I've lost motivation to dig further for the time being. Maybe later.

		Dave

Re: Fix UDF-FS potentially dereferencing null

[viro]

On Sat, Apr 17, 2004 at 12:18:23AM +0100, Dave Jones wrote:
> On Fri, Apr 16, 2004 at 07:13:04PM -0400, Jeff Garzik wrote:
>  > >Check in question is a BS - it never gets NULL passed as dir.
>  > 
>  > Yes, it looks like a lot of these NULL checks caught can be fixed simply 
>  > by removing bogus and/or redundant checks.
> 
> And there's a *lot* of them. Those half dozen or so patches earlier were
> results of just a quick random skim of the list the coverity folks came up with.
> 
> It'll take a lot of effort to 'fix' them all, and given the non-severity
> of a lot of them, I'm not convinced it's worth the effort.

We really ought to kill just-in-case checks like that - they only obfuscate
the code _and_ hide bugs.  The thing we must *not* do is "closing" the
items in that list by adding meaningless precautions of that sort.

Re: Fix UDF-FS potentially dereferencing null

[Linus Torvalds]

On Sat, 17 Apr 2004, Dave Jones wrote:
> 
> And there's a *lot* of them. Those half dozen or so patches earlier were
> results of just a quick random skim of the list the coverity folks came up with.
> 
> It'll take a lot of effort to 'fix' them all, and given the non-severity
> of a lot of them, I'm not convinced it's worth the effort.

Just for the fun of it, I added a "safe" attribute to sparse (hey, it was 
trivial), and made it warn if you test a safe variable. 

You can do

	#define __safe __attribute__((safe))

	static struct denty *
	udf_lookup(struct inode * __safe dir,
			struct dentry * __safe dentry,
			struct nameidata * __safe nd);

or

	int notify_change(struct dentry * dentry, struct iattr * attr)
	{
		struct inode * __safe inode = dentry->d_inode;

and it should actually warn you if you test such a safe variable:

	warning: fs/attr.c:138:6: testing a 'safe expression'

Ehh?

		Linus

Re: Fix UDF-FS potentially dereferencing null

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Sat, 2004-04-17 at 02:44, Linus Torvalds wrote:
> On Sat, 17 Apr 2004, Dave Jones wrote:
> > 
> > And there's a *lot* of them. Those half dozen or so patches earlier were
> > results of just a quick random skim of the list the coverity folks came up with.
> > 
> > It'll take a lot of effort to 'fix' them all, and given the non-severity
> > of a lot of them, I'm not convinced it's worth the effort.
> 
> Just for the fun of it, I added a "safe" attribute to sparse (hey, it was 
> trivial), and made it warn if you test a safe variable. 
> 
> You can do
> 
> 	#define __safe __attribute__((safe))
> 
> 	static struct denty *
> 	udf_lookup(struct inode * __safe dir,
> 			struct dentry * __safe dentry,
> 			struct nameidata * __safe nd);
> 
> or

Hi,

is it maybe a good idea to map this to gcc's "nonnull" attribute in some
way? That way both sparse and the compiler get this explicit
knowledge.... (afaics gcc will then also just optimize out the null ptr
checks)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Fix UDF-FS potentially dereferencing null

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]


> Hi,
> 
> is it maybe a good idea to map this to gcc's "nonnull" attribute in some
> way? That way both sparse and the compiler get this explicit
> knowledge.... (afaics gcc will then also just optimize out the null ptr
> checks)

eh scratch that idea; the nonnull attribute appears to be quite useless
(and unused by gcc)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Fix UDF-FS potentially dereferencing null

[Ingo Oeser]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

On Saturday 17 April 2004 11:50, Arjan van de Ven wrote:
[superflous NULL pointer checks]
> > > It'll take a lot of effort to 'fix' them all, and given the non-severity
> > > of a lot of them, I'm not convinced it's worth the effort.
> > 
> > Just for the fun of it, I added a "safe" attribute to sparse (hey, it was 
> > trivial), and made it warn if you test a safe variable. 
> > 
> > You can do
> > 
> > 	#define __safe __attribute__((safe))
> > 
> > 	static struct denty *
> > 	udf_lookup(struct inode * __safe dir,
> > 			struct dentry * __safe dentry,
> > 			struct nameidata * __safe nd);
> > 
> > or
> is it maybe a good idea to map this to gcc's "nonnull" attribute in some
> way? That way both sparse and the compiler get this explicit
> knowledge.... (afaics gcc will then also just optimize out the null ptr
> checks)

Or even call the attribute "nonnull", because this is a very obvious
naming, even to non-native English readers.

"safe" can mean anything from "safe to use under spinlock" to
"you cannot get pregnant from using this variable".

GCC will not only optimize out the check, but also ensure that the we
will not pass NULL ptrs, if it can notice it. If this gets pushed high
enough (up to the register-like functions, where it gets first
assigned), we will never face this kind of problem anymore and document
this fact per function. Sounds like C coder heaven ;-)


Regards

Ingo Oeser

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFAgRE9U56oYWuOrkARAjQFAKCIvypi8zPswOX/Q4Qlnh01CBrwDQCfTKRQ
BY8VRXpe0ZuHRRIHH8JgDag=
=mQNg
-----END PGP SIGNATURE-----

Re: Fix UDF-FS potentially dereferencing null

[Linus Torvalds]

On Sat, 17 Apr 2004, Ingo Oeser wrote:
> 
> Or even call the attribute "nonnull", because this is a very obvious
> naming, even to non-native English readers.

I did that at first, but decided that what I really wanted was "safe".

"nonnull" is nice for avoiding the NULL check, but it's useless for 
anything else.

"safe" to my mind means that not only is it not NULL, it's also safe to 
dereference early (ie "prefetchable"), which has a lot of meaning for the 
back-end.

> "safe" can mean anything from "safe to use under spinlock" to
> "you cannot get pregnant from using this variable".

That's pretty much _exactly_ what "safe" means.

Basically, a real C compiler is not allowed to dereference a pointer
speculatively, since that could have undefined side effects in the
machine. And "safe" means that there are no side effects, pregnancy- 
or otherwise.

> GCC will not only optimize out the check, but also ensure that the we
> will not pass NULL ptrs, if it can notice it. If this gets pushed high
> enough (up to the register-like functions, where it gets first
> assigned), we will never face this kind of problem anymore and document
> this fact per function. Sounds like C coder heaven ;-)

No. "nonnull" is useless. Even if it isn't NULL, the C standard does not 
allow the compiler to just dereference something willy-nilly.

In contrast, "safe" means that the compiler could do something like

	int * safe ptr;

	if (!a)
		a = *ptr;

and know that it is "safe" to transform this into

	tmp = *ptr;
	a = a ? : tmp;

which it otherwise can't necessarily determine.

		Linus
		

Re: Fix UDF-FS potentially dereferencing null

[Alexandre Oliva]

On Apr 17, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

> On Sat, 17 Apr 2004, Ingo Oeser wrote:
>> 
>> Or even call the attribute "nonnull", because this is a very obvious
>> naming, even to non-native English readers.

> I did that at first, but decided that what I really wanted was "safe".

> "nonnull" is nice for avoiding the NULL check, but it's useless for 
> anything else.

> "safe" to my mind means that not only is it not NULL, it's also safe to 
> dereference early (ie "prefetchable"), which has a lot of meaning for the 
> back-end.

And how far back can this go?

Consider, for example:

inline int foo(int *safe p) {
  return *p;
}

int bar(int *p) {
  if (p)
    return foo(p);
  return -1;
}

I suppose you'd like a compiler to remember the point at which the
pointer became safe, and avoid prefetching it before the test.  So
it's not exactly total freedom to reschedule the load.

Still, this sounds like something that might be useful, especially on
platforms that don't support (non-trapping) prefetching.

GCC's nonnull attribute is indeed useless for these purposes.  Even
though the docs say it could be used to optimize away a NULL test, its
syntax is far too cumbersome, since you apply the nonnull attribute to
the function, not to its argument, which makes it unusable for
non-argument variables.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: Fix UDF-FS potentially dereferencing null

[Linus Torvalds]

On Thu, 22 Apr 2004, Alexandre Oliva wrote:
> 
> > "safe" to my mind means that not only is it not NULL, it's also safe to 
> > dereference early (ie "prefetchable"), which has a lot of meaning for the 
> > back-end.
> 
> And how far back can this go?

Hey, "sparse" is some way away from actually generating code, so I don't
know what the answer will be. From a checking standpoint, it's a fairly
easy attribute: right now it is an attribute on the "pointer", not on the
thing it points to (unlike the "address space" attribute, which means that
the thing the pointer _points_ to is of a different type), and that means
that your example will not cause any warnings (a "safe" pointer and a
normal pointer are compatible as pointers - they both point to the same
thing, so assigning from one to the other is not something we'd warn
about).

>From an actual code generation standpoint, let's see if we ever get there.  
I can see several semantically meaningful barriers ("can't move it past
the assignment of the pointer" or even the very limited "can't move it
past that one syntactic expression"), but it would depend a lot on what
the back-end actually would/could do and keep track of.

In your example, both pointers were called "p", but they were obviously
two different symbols from a compiler perspective. So there's a clear
"assignment" from one "p" to the other "p" as part of the inline function
call, so it's not like the back-end doesn't see that part - it's assigning
from a non-safe pointer to a safe one _after_ doing the test on the
non-safe one.

As such it's perfectly clear to the compiler what is going on in your
example; it's just a practical matter of taking it into account at the
right time. Whether that is a realistic thing to do, and worth doing, I
really don't know.

So I'll just claim that the "safe" attribute at least in theory makes
sense. Whether it matters in practice I'll leave to others.

> GCC's nonnull attribute is indeed useless for these purposes.  Even
> though the docs say it could be used to optimize away a NULL test, its
> syntax is far too cumbersome, since you apply the nonnull attribute to
> the function, not to its argument, which makes it unusable for
> non-argument variables.

Ahh. I didn't even know how the gcc nonnull thing worked. I just knew from
the gcc lists that something like that existed, and I just stupidly
assumed that it was done the sane way (ie the way I did it - which is, of
course, the very definition of "sane" ;).

		Linus

Re: Fix UDF-FS potentially dereferencing null

[Alexandre Oliva]

On Apr 22, 2004, Linus Torvalds <torvalds@osdl.org> wrote:

> In your example, both pointers were called "p", but they were obviously
> two different symbols from a compiler perspective. So there's a clear
> "assignment" from one "p" to the other "p" as part of the inline function
> call, so it's not like the back-end doesn't see that part - it's assigning
> from a non-safe pointer to a safe one _after_ doing the test on the
> non-safe one.

It does see the assignment, yes, but if the pointer happens to be a
constant, and constant propagation turns the assignment `p_i = p;'
into `p_i = constant;', you'd have to preserve the information that
this constant pointer can only be safely dereferenced after the test.
This is an admittedly convoluted example, since if p is constant and
the condition doesn't hold, the conditional dereferencing will
probably have already been optimized away by the time it could do any
damage, but it might not be depending on how the compiler orders its
optimization passes, and then you lose.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}