[PATCH 1/2] scripts: imd: fix uninitialized variable read

[PATCH 1/2] scripts: imd: fix uninitialized variable read

[Ahmad Fatoum]

Errors are propagated in the loop and ret is never set. Remove it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 scripts/bareboximd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/scripts/bareboximd.c b/scripts/bareboximd.c
index b733cae61c87..d11b661fa391 100644
--- a/scripts/bareboximd.c
+++ b/scripts/bareboximd.c
@@ -53,7 +53,7 @@ int imd_command_setenv(const char *variable_name, const char *value)
 
 static int write_file(const char *filename, const void *buf, size_t size)
 {
-	int fd, ret;
+	int fd;
 	int now;
 
 	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
@@ -74,9 +74,6 @@ static int write_file(const char *filename, const void *buf, size_t size)
 
 	close(fd);
 
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
-- 
2.26.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] of: fix side effect in macro expansion

[Ahmad Fatoum]

On little endian platforms that don't define __arch_swab32p, be32_to_cpu
ultimately expands to an expression that evaluates its argument multiple
times. Because of the increment, this is wrong (and UB because of
unsequenced modification).
Hoist the side effect out of the macro argument.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/of.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/of.h b/include/of.h
index 85d55f9b57dc..08bbeaf4d21b 100644
--- a/include/of.h
+++ b/include/of.h
@@ -79,8 +79,8 @@ struct fdt_header *of_get_fixed_tree(struct device_node *node);
 static inline u64 of_read_number(const __be32 *cell, int size)
 {
 	u64 r = 0;
-	while (size--)
-		r = (r << 32) | be32_to_cpu(*(cell++));
+	for (; size--; cell++)
+		r = (r << 32) | be32_to_cpu(*cell);
 	return r;
 }
 
-- 
2.26.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] scripts: imd: fix uninitialized variable read

[Sascha Hauer]

On Wed, Apr 29, 2020 at 08:40:39AM +0200, Ahmad Fatoum wrote:
> Errors are propagated in the loop and ret is never set. Remove it.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  scripts/bareboximd.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/scripts/bareboximd.c b/scripts/bareboximd.c
> index b733cae61c87..d11b661fa391 100644
> --- a/scripts/bareboximd.c
> +++ b/scripts/bareboximd.c
> @@ -53,7 +53,7 @@ int imd_command_setenv(const char *variable_name, const char *value)
>  
>  static int write_file(const char *filename, const void *buf, size_t size)
>  {
> -	int fd, ret;
> +	int fd;
>  	int now;
>  
>  	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> @@ -74,9 +74,6 @@ static int write_file(const char *filename, const void *buf, size_t size)
>  
>  	close(fd);
>  
> -	if (ret < 0)
> -		return ret;
> -

There's more wrong in this function. In the error path the file is never
closed. It is not an error when write() returns 0. The return value of
write_file() is never checked by the caller. Steffen, care to fix some
of these?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] of: fix side effect in macro expansion

[Sascha Hauer]

On Wed, Apr 29, 2020 at 08:40:40AM +0200, Ahmad Fatoum wrote:
> On little endian platforms that don't define __arch_swab32p, be32_to_cpu
> ultimately expands to an expression that evaluates its argument multiple
> times. Because of the increment, this is wrong (and UB because of
> unsequenced modification).
> Hoist the side effect out of the macro argument.

I'm not sure if the reasoning is correct. That would mean that
of_read_number() doesn't work on ARM, but I can assure it actually does.

> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  include/of.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/of.h b/include/of.h
> index 85d55f9b57dc..08bbeaf4d21b 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -79,8 +79,8 @@ struct fdt_header *of_get_fixed_tree(struct device_node *node);
>  static inline u64 of_read_number(const __be32 *cell, int size)
>  {
>  	u64 r = 0;
> -	while (size--)
> -		r = (r << 32) | be32_to_cpu(*(cell++));
> +	for (; size--; cell++)
> +		r = (r << 32) | be32_to_cpu(*cell);

That said, the implementation looks way better with this patch.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] of: fix side effect in macro expansion

[Ahmad Fatoum]

On 4/29/20 10:00 AM, Sascha Hauer wrote:
> On Wed, Apr 29, 2020 at 08:40:40AM +0200, Ahmad Fatoum wrote:
>> On little endian platforms that don't define __arch_swab32p, be32_to_cpu
>> ultimately expands to an expression that evaluates its argument multiple
>> times. Because of the increment, this is wrong (and UB because of
>> unsequenced modification).
>> Hoist the side effect out of the macro argument.
> 
> I'm not sure if the reasoning is correct. That would mean that
> of_read_number() doesn't work on ARM, but I can assure it actually does.

Typo, sorry. arch needs to define __arch_swab32 (no p at the end).

of_read_number -> be32_to_cpu -> __be32_to_cpu -> __swab32 -> __fswab32

then

static inline __attribute_const__ __u32 __fswab32(__u32 val)
{
#ifdef __arch_swab32
        return __arch_swab32(val);
#else
        return ___constant_swab32(val);
#endif
}

where ___constant_swab32 is the macro that evaluates the argument
multiple times.

grep shows '#define __arch_swab32' matches for mips, arm, ppc and x86.


> That said, the implementation looks way better with this patch.
Could you amend the commit message when applying?

Thanks
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] of: fix side effect in macro expansion

[Sascha Hauer]

On Wed, Apr 29, 2020 at 10:26:27AM +0200, Ahmad Fatoum wrote:
> On 4/29/20 10:00 AM, Sascha Hauer wrote:
> > On Wed, Apr 29, 2020 at 08:40:40AM +0200, Ahmad Fatoum wrote:
> >> On little endian platforms that don't define __arch_swab32p, be32_to_cpu
> >> ultimately expands to an expression that evaluates its argument multiple
> >> times. Because of the increment, this is wrong (and UB because of
> >> unsequenced modification).
> >> Hoist the side effect out of the macro argument.
> > 
> > I'm not sure if the reasoning is correct. That would mean that
> > of_read_number() doesn't work on ARM, but I can assure it actually does.
> 
> Typo, sorry. arch needs to define __arch_swab32 (no p at the end).
> 
> of_read_number -> be32_to_cpu -> __be32_to_cpu -> __swab32 -> __fswab32
> 
> then
> 
> static inline __attribute_const__ __u32 __fswab32(__u32 val)
> {
> #ifdef __arch_swab32
>         return __arch_swab32(val);
> #else
>         return ___constant_swab32(val);
> #endif
> }
> 
> where ___constant_swab32 is the macro that evaluates the argument
> multiple times.

__fswab32 is a function, for the caller it doesn't matter how often
___constant_swab32 evaluates its argument.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] of: fix side effect in macro expansion

[Ahmad Fatoum]

On 4/29/20 10:41 AM, Sascha Hauer wrote:
> On Wed, Apr 29, 2020 at 10:26:27AM +0200, Ahmad Fatoum wrote:
>> On 4/29/20 10:00 AM, Sascha Hauer wrote:
>>> On Wed, Apr 29, 2020 at 08:40:40AM +0200, Ahmad Fatoum wrote:
>>>> On little endian platforms that don't define __arch_swab32p, be32_to_cpu
>>>> ultimately expands to an expression that evaluates its argument multiple
>>>> times. Because of the increment, this is wrong (and UB because of
>>>> unsequenced modification).
>>>> Hoist the side effect out of the macro argument.
>>>
>>> I'm not sure if the reasoning is correct. That would mean that
>>> of_read_number() doesn't work on ARM, but I can assure it actually does.
>>
>> Typo, sorry. arch needs to define __arch_swab32 (no p at the end).
>>
>> of_read_number -> be32_to_cpu -> __be32_to_cpu -> __swab32 -> __fswab32
>>
>> then
>>
>> static inline __attribute_const__ __u32 __fswab32(__u32 val)
>> {
>> #ifdef __arch_swab32
>>         return __arch_swab32(val);
>> #else
>>         return ___constant_swab32(val);
>> #endif
>> }
>>
>> where ___constant_swab32 is the macro that evaluates the argument
>> multiple times.
> 
> __fswab32 is a function, for the caller it doesn't matter how often
> ___constant_swab32 evaluates its argument.

Oh. Looks like a false positive. cppcheck thinks we could reach
__constant_swab32, which is a macro, but it's only reachable for
constant expressions, which are always side-effect free.

Please dismiss. I'll send a v2 with corrected reasoning in the commit
message.

Cheers
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] scripts: imd: fix uninitialized variable read

[Steffen Trumtrar]

Hi,

Sascha Hauer <s.hauer@pengutronix.de> writes:

> On Wed, Apr 29, 2020 at 08:40:39AM +0200, Ahmad Fatoum wrote:
>> Errors are propagated in the loop and ret is never set. Remove 
>> it.
>> 
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  scripts/bareboximd.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>> 
>> diff --git a/scripts/bareboximd.c b/scripts/bareboximd.c
>> index b733cae61c87..d11b661fa391 100644
>> --- a/scripts/bareboximd.c
>> +++ b/scripts/bareboximd.c
>> @@ -53,7 +53,7 @@ int imd_command_setenv(const char 
>> *variable_name, const char *value)
>>  
>>  static int write_file(const char *filename, const void *buf, 
>>  size_t size)
>>  {
>> -	int fd, ret;
>> +	int fd;
>>  	int now;
>>  
>>  	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR 
>>  | S_IWUSR | S_IRGRP | S_IROTH);
>> @@ -74,9 +74,6 @@ static int write_file(const char *filename, 
>> const void *buf, size_t size)
>>  
>>  	close(fd);
>>  
>> -	if (ret < 0)
>> -		return ret;
>> -
>
> There's more wrong in this function. In the error path the file 
> is never
> closed. It is not an error when write() returns 0. The return 
> value of
> write_file() is never checked by the caller. Steffen, care to 
> fix some
> of these?

yes, I will have a look.


str

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox