platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

PATCH In review Dan Carpenter 5 participants 8 messages Jan 18, 2018 – Jan 19, 2018

Subsystems (2)

Kernel janitors x86 platform drivers

lore

Share & tools

lore

Dan Carpenter

author

2018-01-18 10:45 UTC #0 lore raw

PATCH 1/1 platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()

We're freeing "value_name" which is NULL, so that's a no-op, instead of "location_name" and then we don't free the first zero-th elements of token_location_attrs[] and token_value_attrs[].

Fixes 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens")

Signed-off-by Dan Carpenter <[email protected]>

drivers/platform/x86/dell-smbios.c +2 -2

index 6a60db515bda..d8a21c7ba594 100644
@@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 		continue;
 
 loop_fail_create_value:
-		kfree(value_name);
+		kfree(location_name);
 		goto out_unwind_strings;
 	}
 	smbios_attribute_group.attrs = token_attrs;

@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}

walter harms

2018-01-18 10:50 UTC #1 lore raw

Am 18.01.2018 11:45, schrieb Dan Carpenter:

Quoted 21 lines

We're freeing "value_name" which is NULL, so that's a no-op, instead of "location_name" and then we don't free the first zero-th elements of token_location_attrs[] and token_value_attrs[].
Fixes 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens")
Signed-off-by Dan Carpenter <[email protected]>
drivers/platform/x86/dell-smbios.c +1 -1
index 6a60db515bda..d8a21c7ba594 100644
@@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 		continue;
 
 loop_fail_create_value:
-		kfree(value_name);
+		kfree(location_name);
 		goto out_unwind_strings;
 	}
 	smbios_attribute_group.attrs = token_attrs;

drivers/platform/x86/dell-smbios.c static int build_tokens_sysfs(struct platform_device *dev) · line 529

Quoted 8 lines

drivers/platform/x86/dell-smbios.c +1 -1

@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}

would you mind to reverse order here ?
you know programmers are terrible at couting backwards.

re,
wh

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2018-01-18 11:03 UTC #2 lore raw

Quoted 12 lines · walter harms

drivers/platform/x86/dell-smbios.c static int build_tokens_sysfs(struct platform_device *dev) · line 529
Quoted 9 lines
drivers/platform/x86/dell-smbios.c +1 -1
@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}
would you mind to reverse order here ?
you know programmers are terrible at couting backwards.

I prefer to always unwind in reverse order so I'd prefer to leave it as-is.

regards,
dan carpenter

walter harms

2018-01-18 11:20 UTC #3 lore raw

Am 18.01.2018 12:03, schrieb Dan Carpenter:

Quoted 14 lines · walter harms

Quoted 12 lines
drivers/platform/x86/dell-smbios.c static int build_tokens_sysfs(struct platform_device *dev) · line 529
Quoted 9 lines
drivers/platform/x86/dell-smbios.c +1 -1
@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}
would you mind to reverse order here ?
you know programmers are terrible at couting backwards.

Quoted 2 lines

I prefer to always unwind in reverse order so I'd prefer to leave it as-is.

It is just a comment from my side. we can leave the actual decision to the current maintainer.

re,
wh

Quoted 5 lines

regards,
dan carpenter

Andy Shevchenko

2018-01-18 19:21 UTC #4 lore raw

Quoted 10 lines · walter harms

Am 18.01.2018 12:03, schrieb Dan Carpenter:
Quoted 8 lines · walter harms
Quoted 7 lines
Quoted 7 lines
out_unwind_strings:
drivers/platform/x86/dell-smbios.c +1 -1
-   for (i = i-1; i > 0; i--) {
+   for (i = i-1; i >= 0; i--) {
            kfree(token_location_attrs[i].attr.name);
            kfree(token_value_attrs[i].attr.name);
    }

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
would you mind to reverse order here ?
you know programmers are terrible at couting backwards.
I prefer to always unwind in reverse order so I'd prefer to leave it as-is.
It is just a comment from my side. we can leave the actual decision to the current maintainer.

Right.
And maintainer would like to see simple:

while (i--) {
    kfree(token_location_attrs[i].attr.name);
    kfree(token_value_attrs[i].attr.name);
}

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2018-01-18 19:24 UTC #5 lore raw

Walter, please, do not shrink Cc list until you sure person / people in question is/are not interested in the topic.

Quoted 31 lines · Andy Shevchenko

Quoted 10 lines · walter harms
Am 18.01.2018 12:03, schrieb Dan Carpenter:
Quoted 8 lines · walter harms
Quoted 7 lines
Quoted 7 lines
out_unwind_strings:
drivers/platform/x86/dell-smbios.c +1 -1
-   for (i = i-1; i > 0; i--) {
+   for (i = i-1; i >= 0; i--) {
            kfree(token_location_attrs[i].attr.name);
            kfree(token_value_attrs[i].attr.name);
    }
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
would you mind to reverse order here ?
you know programmers are terrible at couting backwards.
I prefer to always unwind in reverse order so I'd prefer to leave it as-is.
It is just a comment from my side. we can leave the actual decision to the current maintainer.
Right.
And maintainer would like to see simple:
while (i--) {
    kfree(token_location_attrs[i].attr.name);
    kfree(token_value_attrs[i].attr.name);
}
--
With Best Regards,
Andy Shevchenko

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Pali Rohár

2018-01-19 23:59 UTC #6 lore raw

Quoted 4 lines · On Thursday 18 January 2018 21:24:44 Andy Shevchenko wrote:

Walter, please, do not shrink Cc list until you sure person / people in question is/are not interested in the topic.

Quoted 29 lines

Quoted 27 lines · Andy Shevchenko
Quoted 10 lines · walter harms
Am 18.01.2018 12:03, schrieb Dan Carpenter:
Quoted 8 lines · walter harms
Quoted 7 lines
Quoted 7 lines
out_unwind_strings:
drivers/platform/x86/dell-smbios.c +1 -1
-   for (i = i-1; i > 0; i--) {
+   for (i = i-1; i >= 0; i--) {
            kfree(token_location_attrs[i].attr.name);
            kfree(token_value_attrs[i].attr.name);
    }
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
would you mind to reverse order here ?
you know programmers are terrible at couting backwards.
I prefer to always unwind in reverse order so I'd prefer to leave it as-is.
It is just a comment from my side. we can leave the actual decision to the current maintainer.
Right.
And maintainer would like to see simple:
while (i--) {
    kfree(token_location_attrs[i].attr.name);
    kfree(token_value_attrs[i].attr.name);
}

For me this looks better.

Quoted 7 lines

Quoted 4 lines
--
With Best Regards,
Andy Shevchenko

Signature · 6 lines

Pali RohÃ¡r
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[email protected]

Acked-by on v1 1/1

2018-01-19 18:32 UTC #7 lore raw

In reply to Dan Carpenter

Thanks, yeah this is obviously the correct way to do it.

Acked-by Mario Limonciello <[email protected]>

Quoted 34 lines

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Thursday, January 18, 2018 4:45 AM
To: Pali Rohár <[email protected]>; Limonciello, Mario <[email protected]>
Cc: Darren Hart <[email protected]>; Andy Shevchenko <[email protected]>; [email protected]; kernel- [email protected]
Subject: [PATCH] platform/x86: dell-smbios: Fix error handling in build_tokens_sysfs()
We're freeing "value_name" which is NULL, so that's a no-op, instead of "location_name" and then we don't free the first zero-th elements of token_location_attrs[] and token_value_attrs[].
Fixes 33b9ca1e53b4 ("platform/x86: dell-smbios: Add a sysfs interface for SMBIOS
tokens")
Signed-off-by Dan Carpenter <[email protected]>
drivers/platform/x86/dell- renamed from drivers/platform/x86/dell-smbios.c +0 -0
smbios.c
index 6a60db515bda..d8a21c7ba594 100644
drivers/platform/x86/dell-smbios.c +1 -1
@@ -512,7 +512,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 		continue;
 
 loop_fail_create_value:
-		kfree(value_name);
+		kfree(location_name);
 		goto out_unwind_strings;
 	}
 	smbios_attribute_group.attrs = token_attrs;

drivers/platform/x86/dell-smbios.c static int build_tokens_sysfs(struct platform_device *dev) · line 529

Quoted 8 lines

drivers/platform/x86/dell-smbios.c +1 -1

@@ -523,7 +523,7 @@ static int build_tokens_sysfs(struct platform_device *dev)
 	return 0;
 
 out_unwind_strings:
-	for (i = i-1; i > 0; i--) {
+	for (i = i-1; i >= 0; i--) {
 		kfree(token_location_attrs[i].attr.name);
 		kfree(token_value_attrs[i].attr.name);
 	}

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html