[PATCH 0/3] make ntfs_free() NULL safe

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] make ntfs_free() NULL safe

jj (Bugzilla)
Hi

Here's a small series of patches that make it safe to call ntfs_free()
with a NULL pointer and reaps some bennefits from that.

The first patch in the series simply makes ntfs_free() safe to call with a
NULL pointer. This fits with many other kernel freeing functions, that are
generally safe to call with NULL pointers.

The second patch adds some documentation to ntfs_free() similar to what's
already provided for the allocation functions.  ntfs_free() is fairly
simple so you could argue that such documentation is not really needed,
but I say it's still nice to have if for no other reason than
completeness.

The third patch removes a number of tests for NULL pointers before calls
to ntfs_free() that patch 1 make redundant.

This whole things came about because Coverity Prevent spotted that in
fs/ntfs/runlist.c on line 967 we call ntfs_runlists_merge() which frees
its second argument and we then explicitly free that argument via
ntfs_free() again on line 970. This patch series also makes that a non
issue.


--
Jesper Juhl <[hidden email]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3] Make it safe to pass a NULL pointer to ntfs_free()

jj (Bugzilla)
From: Jesper Juhl <[hidden email]>

Signed-off-by: Jesper Juhl <[hidden email]>
---
 fs/ntfs/malloc.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/ntfs/malloc.h b/fs/ntfs/malloc.h
index a44b14c..d833e37 100644
--- a/fs/ntfs/malloc.h
+++ b/fs/ntfs/malloc.h
@@ -85,6 +85,8 @@ static inline void *ntfs_malloc_nofs_nofail(unsigned long size)
 
 static inline void ntfs_free(void *addr)
 {
+ if (!addr)
+ return;
  if (!is_vmalloc_addr(addr)) {
  kfree(addr);
  /* free_page((unsigned long)addr); */
--
1.7.6


--
Jesper Juhl <[hidden email]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3] Add documentation comment for ntfs_free()

jj (Bugzilla)
In reply to this post by jj (Bugzilla)
From: Jesper Juhl <[hidden email]>

Signed-off-by: Jesper Juhl <[hidden email]>
---
 fs/ntfs/malloc.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/ntfs/malloc.h b/fs/ntfs/malloc.h
index d833e37..174408b 100644
--- a/fs/ntfs/malloc.h
+++ b/fs/ntfs/malloc.h
@@ -83,6 +83,12 @@ static inline void *ntfs_malloc_nofs_nofail(unsigned long size)
  return __ntfs_malloc(size, GFP_NOFS | __GFP_HIGHMEM | __GFP_NOFAIL);
 }
 
+/**
+ * ntfs_free - free memory allocated by ntfs_malloc*
+ * @addr: pointer to the memory to be freed
+ *
+ * Frees the memory pointed to by @addr. It is safe to pass a NULL pointer.
+ */
 static inline void ntfs_free(void *addr)
 {
  if (!addr)
--
1.7.6


--
Jesper Juhl <[hidden email]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3] Remove no longer needed NULL checks before calls to ntfs_free()

jj (Bugzilla)
In reply to this post by jj (Bugzilla)
From: Jesper Juhl <[hidden email]>

ntfs_free() can deal with NULL pointers, it's redundant to check first.

Signed-off-by: Jesper Juhl <[hidden email]>
---
 fs/ntfs/inode.c   |   18 ++++++------------
 fs/ntfs/logfile.c |    3 +--
 fs/ntfs/runlist.c |    3 +--
 fs/ntfs/super.c   |   36 ++++++++++++------------------------
 4 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 1371487..bcc298f 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2197,22 +2197,16 @@ static void __ntfs_clear_inode(ntfs_inode *ni)
 {
  /* Free all alocated memory. */
  down_write(&ni->runlist.lock);
- if (ni->runlist.rl) {
- ntfs_free(ni->runlist.rl);
- ni->runlist.rl = NULL;
- }
+ ntfs_free(ni->runlist.rl);
+ ni->runlist.rl = NULL;
  up_write(&ni->runlist.lock);
 
- if (ni->attr_list) {
- ntfs_free(ni->attr_list);
- ni->attr_list = NULL;
- }
+ ntfs_free(ni->attr_list);
+ ni->attr_list = NULL;
 
  down_write(&ni->attr_list_rl.lock);
- if (ni->attr_list_rl.rl) {
- ntfs_free(ni->attr_list_rl.rl);
- ni->attr_list_rl.rl = NULL;
- }
+ ntfs_free(ni->attr_list_rl.rl);
+ ni->attr_list_rl.rl = NULL;
  up_write(&ni->attr_list_rl.lock);
 
  if (ni->name_len && ni->name != I30) {
diff --git a/fs/ntfs/logfile.c b/fs/ntfs/logfile.c
index c71de29..14a9e09 100644
--- a/fs/ntfs/logfile.c
+++ b/fs/ntfs/logfile.c
@@ -651,8 +651,7 @@ is_empty:
  ntfs_debug("Done.");
  return true;
 err_out:
- if (rstr1_ph)
- ntfs_free(rstr1_ph);
+ ntfs_free(rstr1_ph);
  return false;
 }
 
diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
index eac7d67..98aaba9 100644
--- a/fs/ntfs/runlist.c
+++ b/fs/ntfs/runlist.c
@@ -1509,8 +1509,7 @@ int ntfs_rl_truncate_nolock(const ntfs_volume *vol, runlist *const runlist,
  if (!new_length) {
  ntfs_debug("Freeing runlist.");
  runlist->rl = NULL;
- if (rl)
- ntfs_free(rl);
+ ntfs_free(rl);
  return 0;
  }
  if (unlikely(!rl)) {
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index b52706d..03e1bf2 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -2206,10 +2206,8 @@ iput_lcnbmp_err_out:
  iput(vol->lcnbmp_ino);
 iput_attrdef_err_out:
  vol->attrdef_size = 0;
- if (vol->attrdef) {
- ntfs_free(vol->attrdef);
- vol->attrdef = NULL;
- }
+ ntfs_free(vol->attrdef);
+ vol->attrdef = NULL;
 #ifdef NTFS_RW
 iput_upcase_err_out:
 #endif /* NTFS_RW */
@@ -2220,10 +2218,8 @@ iput_upcase_err_out:
  vol->upcase = NULL;
  }
  mutex_unlock(&ntfs_lock);
- if (vol->upcase) {
- ntfs_free(vol->upcase);
- vol->upcase = NULL;
- }
+ ntfs_free(vol->upcase);
+ vol->upcase = NULL;
 iput_mftbmp_err_out:
  iput(vol->mftbmp_ino);
 iput_mirr_err_out:
@@ -2389,10 +2385,8 @@ static void ntfs_put_super(struct super_block *sb)
 
  /* Throw away the table of attribute definitions. */
  vol->attrdef_size = 0;
- if (vol->attrdef) {
- ntfs_free(vol->attrdef);
- vol->attrdef = NULL;
- }
+ ntfs_free(vol->attrdef);
+ vol->attrdef = NULL;
  vol->upcase_len = 0;
  /*
  * Destroy the global default upcase table if necessary.  Also decrease
@@ -2410,10 +2404,8 @@ static void ntfs_put_super(struct super_block *sb)
  if (vol->cluster_size <= 4096 && !--ntfs_nr_compression_users)
  free_compression_buffers();
  mutex_unlock(&ntfs_lock);
- if (vol->upcase) {
- ntfs_free(vol->upcase);
- vol->upcase = NULL;
- }
+ ntfs_free(vol->upcase);
+ vol->upcase = NULL;
 
  unload_nls(vol->nls_map);
 
@@ -2983,10 +2975,8 @@ static int ntfs_fill_super(struct super_block *sb, void *opt, const int silent)
 #endif /* NTFS_RW */
  /* Throw away the table of attribute definitions. */
  vol->attrdef_size = 0;
- if (vol->attrdef) {
- ntfs_free(vol->attrdef);
- vol->attrdef = NULL;
- }
+ ntfs_free(vol->attrdef);
+ vol->attrdef = NULL;
  vol->upcase_len = 0;
  mutex_lock(&ntfs_lock);
  if (vol->upcase == default_upcase) {
@@ -2994,10 +2984,8 @@ static int ntfs_fill_super(struct super_block *sb, void *opt, const int silent)
  vol->upcase = NULL;
  }
  mutex_unlock(&ntfs_lock);
- if (vol->upcase) {
- ntfs_free(vol->upcase);
- vol->upcase = NULL;
- }
+ ntfs_free(vol->upcase);
+ vol->upcase = NULL;
  if (vol->nls_map) {
  unload_nls(vol->nls_map);
  vol->nls_map = NULL;
--
1.7.6


--
Jesper Juhl <[hidden email]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] make ntfs_free() NULL safe

Anton Altaparmakov-2
In reply to this post by jj (Bugzilla)
Hi,

On 19 Aug 2011, at 22:30, Jesper Juhl wrote:

> Here's a small series of patches that make it safe to call ntfs_free()
> with a NULL pointer and reaps some bennefits from that.
>
> The first patch in the series simply makes ntfs_free() safe to call with a
> NULL pointer. This fits with many other kernel freeing functions, that are
> generally safe to call with NULL pointers.
>
> The second patch adds some documentation to ntfs_free() similar to what's
> already provided for the allocation functions.  ntfs_free() is fairly
> simple so you could argue that such documentation is not really needed,
> but I say it's still nice to have if for no other reason than
> completeness.
>
> The third patch removes a number of tests for NULL pointers before calls
> to ntfs_free() that patch 1 make redundant.

Patches look fine.  Feel free to add my

        Acked-by: Anton Altaparmakov <[hidden email]>

and to send them to Linus for inclusion…

> This whole things came about because Coverity Prevent spotted that in
> fs/ntfs/runlist.c on line 967 we call ntfs_runlists_merge() which frees
> its second argument and we then explicitly free that argument via
> ntfs_free() again on line 970. This patch series also makes that a non
> issue.

Ah but Coverity Prevent is incorrect in its spotting!

Have a look yourself!

ntfs_runlists_merge() _ONLY_ frees its second argument if it returns success.  If it returns error it does _NOT_ free its second argument!

And line 970 is _ONLY_ executed if ntfs_runlists_merge() returned error, i.e. in the case that the second argument was _NOT_ freed.  If the argument was freed, ntfs_runlists_merge() would have returned success, and then line 970 would never have been reached…

So I am afraid this is a bug in Coverity Prevent rather than in NTFS.  (-:

Best regards,

        Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] make ntfs_free() NULL safe

jj (Bugzilla)
On Fri, 19 Aug 2011, Anton Altaparmakov wrote:

> Hi,
>
> On 19 Aug 2011, at 22:30, Jesper Juhl wrote:
> > Here's a small series of patches that make it safe to call ntfs_free()
> > with a NULL pointer and reaps some bennefits from that.
> >
> > The first patch in the series simply makes ntfs_free() safe to call with a
> > NULL pointer. This fits with many other kernel freeing functions, that are
> > generally safe to call with NULL pointers.
> >
> > The second patch adds some documentation to ntfs_free() similar to what's
> > already provided for the allocation functions.  ntfs_free() is fairly
> > simple so you could argue that such documentation is not really needed,
> > but I say it's still nice to have if for no other reason than
> > completeness.
> >
> > The third patch removes a number of tests for NULL pointers before calls
> > to ntfs_free() that patch 1 make redundant.
>
> Patches look fine.  Feel free to add my
>
> Acked-by: Anton Altaparmakov <[hidden email]>
>
Thank you.

> and to send them to Linus for inclusion…
>
I think I'll wait a bit before doing that. Hopefully some maintainer will
pick them up and push them. But if that doesn't happen I'll make sure to
re-sumbit them myself and point them higher up the hierarchy (with your
ACK attached) :-)


> > This whole things came about because Coverity Prevent spotted that in
> > fs/ntfs/runlist.c on line 967 we call ntfs_runlists_merge() which frees
> > its second argument and we then explicitly free that argument via
> > ntfs_free() again on line 970. This patch series also makes that a non
> > issue.
>
> Ah but Coverity Prevent is incorrect in its spotting!
>
> Have a look yourself!
>
> ntfs_runlists_merge() _ONLY_ frees its second argument if it returns success.  If it returns error it does _NOT_ free its second argument!
>
On second inspection I believe you are right.

> And line 970 is _ONLY_ executed if ntfs_runlists_merge() returned error, i.e. in the case that the second argument was _NOT_ freed.  If the argument was freed, ntfs_runlists_merge() would have returned success, and then line 970 would never have been reached…
>
> So I am afraid this is a bug in Coverity Prevent rather than in NTFS.  (-:
>
I'll make a note in prevent that this is a false positive.

I still believe the 3 patches make sense though, regardless of this.

--
Jesper Juhl <[hidden email]>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/3] make ntfs_free() NULL safe

Anton Altaparmakov-2
Hi,

On 19 Aug 2011, at 23:21, Jesper Juhl wrote:

> On Fri, 19 Aug 2011, Anton Altaparmakov wrote:
>> On 19 Aug 2011, at 22:30, Jesper Juhl wrote:
>>> Here's a small series of patches that make it safe to call ntfs_free()
>>> with a NULL pointer and reaps some bennefits from that.
>>>
>>> The first patch in the series simply makes ntfs_free() safe to call with a
>>> NULL pointer. This fits with many other kernel freeing functions, that are
>>> generally safe to call with NULL pointers.
>>>
>>> The second patch adds some documentation to ntfs_free() similar to what's
>>> already provided for the allocation functions.  ntfs_free() is fairly
>>> simple so you could argue that such documentation is not really needed,
>>> but I say it's still nice to have if for no other reason than
>>> completeness.
>>>
>>> The third patch removes a number of tests for NULL pointers before calls
>>> to ntfs_free() that patch 1 make redundant.
>>
>> Patches look fine.  Feel free to add my
>>
>> Acked-by: Anton Altaparmakov <[hidden email]>
>>
> Thank you.
>
>> and to send them to Linus for inclusion…
>>
> I think I'll wait a bit before doing that. Hopefully some maintainer will
> pick them up and push them. But if that doesn't happen I'll make sure to
> re-sumbit them myself and point them higher up the hierarchy (with your
> ACK attached) :-)

Ok, thanks.  I have no other changes for NTFS so no point in me taking them to submit to Linus, you might as well do it yourself (and I am incredibly busy at the moment).  (-:

>>> This whole things came about because Coverity Prevent spotted that in
>>> fs/ntfs/runlist.c on line 967 we call ntfs_runlists_merge() which frees
>>> its second argument and we then explicitly free that argument via
>>> ntfs_free() again on line 970. This patch series also makes that a non
>>> issue.
>>
>> Ah but Coverity Prevent is incorrect in its spotting!
>>
>> Have a look yourself!
>>
>> ntfs_runlists_merge() _ONLY_ frees its second argument if it returns success.  If it returns error it does _NOT_ free its second argument!
>>
> On second inspection I believe you are right.

(-:

>> And line 970 is _ONLY_ executed if ntfs_runlists_merge() returned error, i.e. in the case that the second argument was _NOT_ freed.  If the argument was freed, ntfs_runlists_merge() would have returned success, and then line 970 would never have been reached…
>>
>> So I am afraid this is a bug in Coverity Prevent rather than in NTFS.  (-:
>>
> I'll make a note in prevent that this is a false positive.
>
> I still believe the 3 patches make sense though, regardless of this.

Indeed, that is why you got my ACK for them…  (-:

Best regards,

        Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/


------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2
_______________________________________________
Linux-NTFS-Dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/linux-ntfs-dev