* [PATCH] smbios: Fix table when no string is present
@ 2021-03-17 11:30 matthias.bgg at kernel.org
2021-03-25 0:38 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: matthias.bgg at kernel.org @ 2021-03-17 11:30 UTC (permalink / raw)
To: u-boot
From: Matthias Brugger <mbrugger@suse.com>
When no string is present in a table, next_ptr points to the same
location as eos. When calculating the string table length, we would only
reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no
strings a present.
Signed-off-by: Matthias Brugger <mbrugger@suse.com>
---
lib/smbios.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/smbios.c b/lib/smbios.c
index 7d463c84a9..d21d37cdac 100644
--- a/lib/smbios.c
+++ b/lib/smbios.c
@@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
{
ctx->eos = eos;
- ctx->next_ptr = eos;
+ ctx->next_ptr = eos + 1;
ctx->last_str = NULL;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] smbios: Fix table when no string is present
2021-03-17 11:30 [PATCH] smbios: Fix table when no string is present matthias.bgg at kernel.org
@ 2021-03-25 0:38 ` Simon Glass
2021-03-25 3:38 ` Matthias Brugger
0 siblings, 1 reply; 4+ messages in thread
From: Simon Glass @ 2021-03-25 0:38 UTC (permalink / raw)
To: u-boot
Hi Matthias,
On Thu, 18 Mar 2021 at 00:30, <matthias.bgg@kernel.org> wrote:
>
> From: Matthias Brugger <mbrugger@suse.com>
>
> When no string is present in a table, next_ptr points to the same
> location as eos. When calculating the string table length, we would only
> reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no
> strings a present.
>
> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>
> ---
>
> lib/smbios.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
There is a bug here but I don't think this is right fix. I remember
worrying about this, making the same change as you did, reverting it
and then forgetting about it :-( It has no tests.
You are effectively changing the definition of next_ptr here:
* @next_ptr: pointer to the start of the next string to be added. When the
* table is not empty, this points to the byte after the \0 of the
* previous string.
(there is a typo in that in the code)
I think that breaks adding new strings.
Can you instead change smbios_string_table_len() to add 2?
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 7d463c84a9..d21d37cdac 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> {
> ctx->eos = eos;
> - ctx->next_ptr = eos;
> + ctx->next_ptr = eos + 1;
> ctx->last_str = NULL;
> }
>
> --
> 2.30.2
>
Regards,
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] smbios: Fix table when no string is present
2021-03-25 0:38 ` Simon Glass
@ 2021-03-25 3:38 ` Matthias Brugger
2021-04-21 6:36 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: Matthias Brugger @ 2021-03-25 3:38 UTC (permalink / raw)
To: u-boot
On 25/03/2021 01:38, Simon Glass wrote:
> Hi Matthias,
>
> On Thu, 18 Mar 2021 at 00:30, <matthias.bgg@kernel.org> wrote:
>>
>> From: Matthias Brugger <mbrugger@suse.com>
>>
>> When no string is present in a table, next_ptr points to the same
>> location as eos. When calculating the string table length, we would only
>> reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no
>> strings a present.
>>
>> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
>>
>> ---
>>
>> lib/smbios.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> There is a bug here but I don't think this is right fix. I remember
> worrying about this, making the same change as you did, reverting it
> and then forgetting about it :-( It has no tests.
>
> You are effectively changing the definition of next_ptr here:
>
> * @next_ptr: pointer to the start of the next string to be added. When the
> * table is not empty, this points to the byte after the \0 of the
> * previous string.
>
That's true.
> (there is a typo in that in the code)
>
> I think that breaks adding new strings.
>
Well it doesn't because when adding a new string, ctx->next_ptr gets overwritten
[1]. It is only used to calculate the lenght of the string in
smbios_string_table_len() to calculate the size of the table [2]. But yes it's
not the obvious way to handle the case when no string is present.
> Can you instead change smbios_string_table_len() to add 2?
>
Do you mean returning 2 when no string is present (ctx->next_ptr == ctx->eos)?
Adding 2 will break the case when we have a string present, as a string already
finishes with '\0' and we only need a second '\0'.
Regards,
Matthias
[1] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L95
[2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L196
>>
>> diff --git a/lib/smbios.c b/lib/smbios.c
>> index 7d463c84a9..d21d37cdac 100644
>> --- a/lib/smbios.c
>> +++ b/lib/smbios.c
>> @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
>> static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
>> {
>> ctx->eos = eos;
>> - ctx->next_ptr = eos;
>> + ctx->next_ptr = eos + 1;
>> ctx->last_str = NULL;
>> }
>>
>> --
>> 2.30.2
>>
>
> Regards,
> Simon
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] smbios: Fix table when no string is present
2021-03-25 3:38 ` Matthias Brugger
@ 2021-04-21 6:36 ` Simon Glass
0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2021-04-21 6:36 UTC (permalink / raw)
To: u-boot
Hi Matthias,
On Thu, 25 Mar 2021 at 16:38, Matthias Brugger <matthias.bgg@gmail.com> wrote:
>
>
>
> On 25/03/2021 01:38, Simon Glass wrote:
> > Hi Matthias,
> >
> > On Thu, 18 Mar 2021 at 00:30, <matthias.bgg@kernel.org> wrote:
> >>
> >> From: Matthias Brugger <mbrugger@suse.com>
> >>
> >> When no string is present in a table, next_ptr points to the same
> >> location as eos. When calculating the string table length, we would only
> >> reserve one \0. By spec a SMBIOS table has to end with two \0\0 when no
> >> strings a present.
> >>
> >> Signed-off-by: Matthias Brugger <mbrugger@suse.com>
> >>
> >> ---
> >>
> >> lib/smbios.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > There is a bug here but I don't think this is right fix. I remember
> > worrying about this, making the same change as you did, reverting it
> > and then forgetting about it :-( It has no tests.
> >
> > You are effectively changing the definition of next_ptr here:
> >
> > * @next_ptr: pointer to the start of the next string to be added. When the
> > * table is not empty, this points to the byte after the \0 of the
> > * previous string.
> >
>
> That's true.
>
> > (there is a typo in that in the code)
> >
> > I think that breaks adding new strings.
> >
>
> Well it doesn't because when adding a new string, ctx->next_ptr gets overwritten
> [1]. It is only used to calculate the lenght of the string in
> smbios_string_table_len() to calculate the size of the table [2]. But yes it's
> not the obvious way to handle the case when no string is present.
>
> > Can you instead change smbios_string_table_len() to add 2?
> >
>
> Do you mean returning 2 when no string is present (ctx->next_ptr == ctx->eos)?
>
> Adding 2 will break the case when we have a string present, as a string already
> finishes with '\0' and we only need a second '\0'.
Oh dear, this is complicated. I think we need to add a few unit tests.
Do you have time to try that?
Regards,
Simon
>
> Regards,
> Matthias
>
> [1] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L95
> [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/smbios.c#L196
>
> >>
> >> diff --git a/lib/smbios.c b/lib/smbios.c
> >> index 7d463c84a9..d21d37cdac 100644
> >> --- a/lib/smbios.c
> >> +++ b/lib/smbios.c
> >> @@ -153,7 +153,7 @@ static int smbios_add_prop(struct smbios_ctx *ctx, const char *prop)
> >> static void smbios_set_eos(struct smbios_ctx *ctx, char *eos)
> >> {
> >> ctx->eos = eos;
> >> - ctx->next_ptr = eos;
> >> + ctx->next_ptr = eos + 1;
> >> ctx->last_str = NULL;
> >> }
> >>
> >> --
> >> 2.30.2
> >>
> >
> > Regards,
> > Simon
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-21 6:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 11:30 [PATCH] smbios: Fix table when no string is present matthias.bgg at kernel.org
2021-03-25 0:38 ` Simon Glass
2021-03-25 3:38 ` Matthias Brugger
2021-04-21 6:36 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.