Message ID | 20210317113045.8074-1-matthias.bgg@kernel.org |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
Series | smbios: Fix table when no string is present | expand |
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
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 >
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 > >
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; }