diff mbox

[01/13] btrfs-progs: btrfs-image.c: Added NULL pointer check.

Message ID 1358715858-4469-2-git-send-email-gene@czarc.net (mailing list archive)
State New, archived
Headers show

Commit Message

Gene Czarcinski Jan. 20, 2013, 9:04 p.m. UTC 
From: Nageswara R Sastry <nasastry@in.ibm.com>

Check for the return value of 'open_ctree()' before dereferencing it.

Signed-off-by: Nageswara R Sastry <nasastry@in.ibm.com>
Signed-off-by: Gene Czarcinski <gene@czarc.net>
---
 btrfs-image.c | 1 +
 1 file changed, 1 insertion(+)

Comments

David Sterba Jan. 21, 2013, 3:56 p.m. UTC | #1 
On Sun, Jan 20, 2013 at 04:04:06PM -0500, Gene Czarcinski wrote:
> From: Nageswara R Sastry <nasastry@in.ibm.com>
> 
> Check for the return value of 'open_ctree()' before dereferencing it.
> 
> --- a/btrfs-image.c
> +++ b/btrfs-image.c
> @@ -491,6 +491,7 @@ static int create_metadump(const char *input, FILE *out, int num_threads,
>  	int ret;
>  
>  	root = open_ctree(input, 0, 0);
> +	BUG_ON(!root);

Bug_on is not the right fix here, I prefer a more extensive fix

http://permalink.gmane.org/gmane.comp.file-systems.btrfs/15305

which prints a message and exits.

>  	BUG_ON(root->nodesize != root->leafsize);
>  
>  	ret = metadump_init(&metadump, root, out, num_threads,
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gene Czarcinski Jan. 22, 2013, 5:34 p.m. UTC | #2 
On 01/21/2013 10:56 AM, David Sterba wrote:
> On Sun, Jan 20, 2013 at 04:04:06PM -0500, Gene Czarcinski wrote:
>> From: Nageswara R Sastry <nasastry@in.ibm.com>
>>
>> Check for the return value of 'open_ctree()' before dereferencing it.
>>
>> --- a/btrfs-image.c
>> +++ b/btrfs-image.c
>> @@ -491,6 +491,7 @@ static int create_metadump(const char *input, FILE *out, int num_threads,
>>   	int ret;
>>   
>>   	root = open_ctree(input, 0, 0);
>> +	BUG_ON(!root);
> Bug_on is not the right fix here, I prefer a more extensive fix
>
> http://permalink.gmane.org/gmane.comp.file-systems.btrfs/15305
>
> which prints a message and exits.
>
>>   	BUG_ON(root->nodesize != root->leafsize);
>>   
>>   	ret = metadump_init(&metadump, root, out, num_threads,
I missed this better fix because I only started looking for outstanding 
patches starting in June/July 2012.

Besides, I suspect you are a lot more knowledgeable to make the judgment 
as to a "good" fix.

Gene
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Jan. 23, 2013, 4:35 p.m. UTC | #3 
On Tue, Jan 22, 2013 at 12:34:01PM -0500, Gene Czarcinski wrote:
> I missed this better fix because I only started looking for outstanding
> patches starting in June/July 2012.

No problem, there are multuple patches for some longstanging bugs, or
several versions of patch series. This is something we need to be able
to deal with anyway.

david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gene Czarcinski Jan. 23, 2013, 6:16 p.m. UTC | #4 
On 01/21/2013 10:56 AM, David Sterba wrote:
> On Sun, Jan 20, 2013 at 04:04:06PM -0500, Gene Czarcinski wrote:
>> >From: Nageswara R Sastry<nasastry@in.ibm.com>
>> >
>> >Check for the return value of 'open_ctree()' before dereferencing it.
>> >
>> >--- a/btrfs-image.c
>> >+++ b/btrfs-image.c
>> >@@ -491,6 +491,7 @@ static int create_metadump(const char *input, FILE *out, int num_threads,
>> >  	int ret;
>> >  
>> >  	root = open_ctree(input, 0, 0);
>> >+	BUG_ON(!root);
> Bug_on is not the right fix here, I prefer a more extensive fix
>
> http://permalink.gmane.org/gmane.comp.file-systems.btrfs/15305
>
> which prints a message and exits.
>
The above patch is 1 of 4 that were submitted.  Should those other three 
be included also.  I did a couple of spot checks and the changes were 
not picked up in another patch.

A couple of things bother me about these 4 patches.  First of all, 
nobody commented about them on this mailing list --- either good or 
bad.  Second, if the descriptions and code is at all accurate, why were 
these patches not included in the "flury" of patches applied later in 
the year to bring btrfs-progs more up-to-date?

These patches may "look good" but almost none of the changes will apply 
to "integration-20130121" or "91d9eec".
So some effort will be needed to refit/rebase these patches.  I am 
willing to do that if there is some agreement that this should be done.  
Of the 4 patches, the one addressing segfaults is the most serious.  The 
others deal with simple problems including one which changes spaces to 
tab characters.

Gene
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/btrfs-image.c b/btrfs-image.c
index f2bbcc8..2a33a55 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -491,6 +491,7 @@  static int create_metadump(const char *input, FILE *out, int num_threads,
 	int ret;
 
 	root = open_ctree(input, 0, 0);
+	BUG_ON(!root);
 	BUG_ON(root->nodesize != root->leafsize);
 
 	ret = metadump_init(&metadump, root, out, num_threads,