diff mbox series

[libubootenv,RFC] uboot_env: Emulate %ms in sscanf()

Message ID E32EC0F1-3CEB-45E7-9CB5-92DB031D4B92@siemens.com
State Changes Requested
Headers show
Series [libubootenv,RFC] uboot_env: Emulate %ms in sscanf() | expand

Commit Message

Storm, Christian Nov. 29, 2023, 8:51 p.m. UTC 
Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 src/uboot_env.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Comments

Michael Heimpold Nov. 30, 2023, 2:27 p.m. UTC | #1 
Hi,

Am Mittwoch, 29. November 2023, 21:51:48 CET schrieb 'Storm, Christian' via 
swupdate:
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>  src/uboot_env.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/uboot_env.c b/src/uboot_env.c
> index bb14cab..7d3264e 100644
> --- a/src/uboot_env.c
> +++ b/src/uboot_env.c
> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx
> **ctxlist, const char *config) ctx->size = 0;
>  	rewind(fp);
> 
> -	while (getline(&line, &bufsize, fp) != -1) {
> +	int len;
> +	while ((len = getline(&line, &bufsize, fp)) != -1) {
>  		/* skip comments */
>  		if (line[0] == '#')
>  			continue;
> 
> +#if defined(__FreeBSD__)
> +		/*
> +		 * POSIX.1-2008 introduced the dynamic allocation 
conversion
> +		 * specifier %m which is not implemented on FreeBSD.
> +		 */
> +		tmp = calloc(1, len + 1);
> +		ret = sscanf(line, "%s %lli %zx %zx %lx %d",
> +				tmp,
> +#else
> +		(void)len;
>  		ret = sscanf(line, "%ms %lli %zx %zx %lx %d",
>  				&tmp,
> +#endif
>  				&dev->offset,
>  				&dev->envsize,
>  				&dev->sectorsize,
> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx
> **ctxlist, const char *config) /*
>  		 * At least name offset and size should be set
>  		 */
> -		if (ret < 3 || !tmp)
> +		if (ret < 3) {
> +			if (tmp) {
> +				free(tmp);
> +			}

Usually free() should accept NULL pointers and do nothing in this case, so the 
"if" could be probably omitted.
Same for the change below.

Best regards,
Michael

>  			continue;
> +		}
> 
>  		/*
>  		 * If size is set but zero, entry is wrong
>  		 */
>  		if (!dev->envsize) {
> +			if (tmp) {
> +				free(tmp);
> +			}
>  			retval = -EINVAL;
>  			break;
>  		}
Storm, Christian Nov. 30, 2023, 2:42 p.m. UTC | #2 
Hi Michael,

>> Signed-off-by: Christian Storm <christian.storm@siemens.com>
>> ---
>> src/uboot_env.c | 23 +++++++++++++++++++++--
>> 1 file changed, 21 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/uboot_env.c b/src/uboot_env.c
>> index bb14cab..7d3264e 100644
>> --- a/src/uboot_env.c
>> +++ b/src/uboot_env.c
>> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx
>> **ctxlist, const char *config) ctx->size = 0;
>> rewind(fp);
>> 
>> - while (getline(&line, &bufsize, fp) != -1) {
>> + int len;
>> + while ((len = getline(&line, &bufsize, fp)) != -1) {
>> /* skip comments */
>> if (line[0] == '#')
>> continue;
>> 
>> +#if defined(__FreeBSD__)
>> + /*
>> + * POSIX.1-2008 introduced the dynamic allocation
> conversion
>> + * specifier %m which is not implemented on FreeBSD.
>> + */
>> + tmp = calloc(1, len + 1);
>> + ret = sscanf(line, "%s %lli %zx %zx %lx %d",
>> + tmp,
>> +#else
>> + (void)len;
>> ret = sscanf(line, "%ms %lli %zx %zx %lx %d",
>> &tmp,
>> +#endif
>> &dev->offset,
>> &dev->envsize,
>> &dev->sectorsize,
>> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx
>> **ctxlist, const char *config) /*
>> * At least name offset and size should be set
>> */
>> - if (ret < 3 || !tmp)
>> + if (ret < 3) {
>> + if (tmp) {
>> + free(tmp);
>> + }
> 
> Usually free() should accept NULL pointers and do nothing in this case, so the 
> "if" could be probably omitted.
> Same for the change below.

Emphasis should be on "usually" :) That depends on the libc implementation...
Either way, I did this consciously for this reason and, more importantly, for being explicit that there maybe is something to free() which is more apparent this way.

But I don't care too much if you prefer a shorter version...


Kind regards,
  Christian
Michael Heimpold Nov. 30, 2023, 5:23 p.m. UTC | #3 
Hi Christian,

Am Donnerstag, 30. November 2023, 15:42:41 CET schrieb Storm, Christian:
> Hi Michael,
> 
> >> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> >> ---
> >> src/uboot_env.c | 23 +++++++++++++++++++++--
> >> 1 file changed, 21 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/uboot_env.c b/src/uboot_env.c
> >> index bb14cab..7d3264e 100644
> >> --- a/src/uboot_env.c
> >> +++ b/src/uboot_env.c
> >> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx
> >> **ctxlist, const char *config) ctx->size = 0;
> >> rewind(fp);
> >> 
> >> - while (getline(&line, &bufsize, fp) != -1) {
> >> + int len;
> >> + while ((len = getline(&line, &bufsize, fp)) != -1) {
> >> /* skip comments */
> >> if (line[0] == '#')
> >> continue;
> >> 
> >> +#if defined(__FreeBSD__)
> >> + /*
> >> + * POSIX.1-2008 introduced the dynamic allocation
> > 
> > conversion
> > 
> >> + * specifier %m which is not implemented on FreeBSD.
> >> + */
> >> + tmp = calloc(1, len + 1);
> >> + ret = sscanf(line, "%s %lli %zx %zx %lx %d",
> >> + tmp,
> >> +#else
> >> + (void)len;
> >> ret = sscanf(line, "%ms %lli %zx %zx %lx %d",
> >> &tmp,
> >> +#endif
> >> &dev->offset,
> >> &dev->envsize,
> >> &dev->sectorsize,
> >> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx
> >> **ctxlist, const char *config) /*
> >> * At least name offset and size should be set
> >> */
> >> - if (ret < 3 || !tmp)
> >> + if (ret < 3) {
> >> + if (tmp) {
> >> + free(tmp);
> >> + }
> > 
> > Usually free() should accept NULL pointers and do nothing in this case, so
> > the "if" could be probably omitted.
> > Same for the change below.
> 
> Emphasis should be on "usually" :) That depends on the libc
> implementation... Either way, I did this consciously for this reason and,
> more importantly, for being explicit that there maybe is something to
> free() which is more apparent this way.

I also remember these days, especially on older embedded systems/compilers...
but AFAIK this is defined in the C standard (or POSIX or whatever) and
thus having a check for something what is expected behavior is considered
bad practise on my personal side and code reviewers should nowadays be aware 
of this - you might have another opinion so please don't feel offended :-)
And I'm especially not the person who decides here.

Kind regards,
Michael
> 
> But I don't care too much if you prefer a shorter version...
> 
> 
> Kind regards,
>   Christian
Stefano Babic Nov. 30, 2023, 8:08 p.m. UTC | #4 
Hi,

On 30.11.23 18:23, Michael Heimpold wrote:
> Hi Christian,
> 
> Am Donnerstag, 30. November 2023, 15:42:41 CET schrieb Storm, Christian:
>> Hi Michael,
>>
>>>> Signed-off-by: Christian Storm <christian.storm@siemens.com>
>>>> ---
>>>> src/uboot_env.c | 23 +++++++++++++++++++++--
>>>> 1 file changed, 21 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/uboot_env.c b/src/uboot_env.c
>>>> index bb14cab..7d3264e 100644
>>>> --- a/src/uboot_env.c
>>>> +++ b/src/uboot_env.c
>>>> @@ -1258,13 +1258,25 @@ int libuboot_read_config_ext(struct uboot_ctx
>>>> **ctxlist, const char *config) ctx->size = 0;
>>>> rewind(fp);
>>>>
>>>> - while (getline(&line, &bufsize, fp) != -1) {
>>>> + int len;
>>>> + while ((len = getline(&line, &bufsize, fp)) != -1) {
>>>> /* skip comments */
>>>> if (line[0] == '#')
>>>> continue;
>>>>
>>>> +#if defined(__FreeBSD__)
>>>> + /*
>>>> + * POSIX.1-2008 introduced the dynamic allocation
>>>
>>> conversion
>>>
>>>> + * specifier %m which is not implemented on FreeBSD.
>>>> + */
>>>> + tmp = calloc(1, len + 1);
>>>> + ret = sscanf(line, "%s %lli %zx %zx %lx %d",
>>>> + tmp,
>>>> +#else
>>>> + (void)len;
>>>> ret = sscanf(line, "%ms %lli %zx %zx %lx %d",
>>>> &tmp,
>>>> +#endif
>>>> &dev->offset,
>>>> &dev->envsize,
>>>> &dev->sectorsize,
>>>> @@ -1274,13 +1286,20 @@ int libuboot_read_config_ext(struct uboot_ctx
>>>> **ctxlist, const char *config) /*
>>>> * At least name offset and size should be set
>>>> */
>>>> - if (ret < 3 || !tmp)
>>>> + if (ret < 3) {
>>>> + if (tmp) {
>>>> + free(tmp);
>>>> + }
>>>
>>> Usually free() should accept NULL pointers and do nothing in this case, so
>>> the "if" could be probably omitted.
>>> Same for the change below.
>>
>> Emphasis should be on "usually" :) That depends on the libc
>> implementation... Either way, I did this consciously for this reason and,
>> more importantly, for being explicit that there maybe is something to
>> free() which is more apparent this way.
> 
> I also remember these days, especially on older embedded systems/compilers...

..right...

> but AFAIK this is defined in the C standard (or POSIX or whatever) and
> thus having a check for something what is expected behavior is considered
> bad practise on my personal side and code reviewers should nowadays be aware
> of this - you might have another opinion so please don't feel offended :-)
> And I'm especially not the person who decides here.

Sure we have not the same behavior in all code, but we have already in 
some cases decided in previous patch to remove the check because we rely 
on free() behavior.

Best regards,
Stefano

> 
> Kind regards,
> Michael
>>
>> But I don't care too much if you prefer a shorter version...
>>
>>
>> Kind regards,
>>    Christian
> 
> 
> 
>
diff mbox series

Patch

diff --git a/src/uboot_env.c b/src/uboot_env.c
index bb14cab..7d3264e 100644
--- a/src/uboot_env.c
+++ b/src/uboot_env.c
@@ -1258,13 +1258,25 @@  int libuboot_read_config_ext(struct uboot_ctx **ctxlist, const char *config)
 	ctx->size = 0;
 	rewind(fp);
 
-	while (getline(&line, &bufsize, fp) != -1) {
+	int len;
+	while ((len = getline(&line, &bufsize, fp)) != -1) {
 		/* skip comments */
 		if (line[0] == '#')
 			continue;
 
+#if defined(__FreeBSD__)
+		/*
+		 * POSIX.1-2008 introduced the dynamic allocation conversion
+		 * specifier %m which is not implemented on FreeBSD.
+		 */
+		tmp = calloc(1, len + 1);
+		ret = sscanf(line, "%s %lli %zx %zx %lx %d",
+				tmp,
+#else
+		(void)len;
 		ret = sscanf(line, "%ms %lli %zx %zx %lx %d",
 				&tmp,
+#endif
 				&dev->offset,
 				&dev->envsize,
 				&dev->sectorsize,
@@ -1274,13 +1286,20 @@  int libuboot_read_config_ext(struct uboot_ctx **ctxlist, const char *config)
 		/*
 		 * At least name offset and size should be set
 		 */
-		if (ret < 3 || !tmp)
+		if (ret < 3) {
+			if (tmp) {
+				free(tmp);
+			}
 			continue;
+		}
 
 		/*
 		 * If size is set but zero, entry is wrong
 		 */
 		if (!dev->envsize) {
+			if (tmp) {
+				free(tmp);
+			}
 			retval = -EINVAL;
 			break;
 		}