From mboxrd@z Thu Jan  1 00:00:00 1970
Subject: Re: [edk2-devel] [PATCH v1 1/1] DynamicTablesPkg: Add SMBIOS String table helper library
To: PierreGondois <pierre.gondois@arm.com>,devel@edk2.groups.io
From: "Sami Mujawar" <sami.mujawar@arm.com>
X-Originating-Location: Cambridge, England, GB (217.140.106.49)
X-Originating-Platform: Mac Firefox 110
User-Agent: GROUPS.IO Web Poster
MIME-Version: 1.0
Date: Thu, 09 Mar 2023 03:11:28 -0800
References: <61170181-b9e3-cb57-a439-47f0d816bf04@arm.com>
In-Reply-To: <61170181-b9e3-cb57-a439-47f0d816bf04@arm.com>
Message-ID: <8800.1678360288123253286@groups.io>
Content-Type: multipart/alternative; boundary="h5K6MnLOoGCCBUjtH0Tr"

--h5K6MnLOoGCCBUjtH0Tr
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Pierre,

On Tue, Oct 4, 2022 at 01:22 AM, PierreGondois wrote:

>=20
>=20
>>=20
>>> +EFI_STATUS
>>> +EFIAPI
>>> +StringTablePublishStringSet (
>>> + IN STRING_TABLE *CONST StrTable,
>>> + IN CHAR8 *CONST SmbiosStringAreaStart,
>>> + IN CONST UINTN SmbiosStringAreaSize
>>> + )
>>> +{
>>> + UINT8 Index;
>>> + STRING_ELEMENT *StrElement;
>>> + CHAR8 *SmbiosString;
>>> + UINTN BytesRemaining;
>>> + UINTN BytesCopied;
>>> +
>>> + if ((StrTable =3D=3D NULL) || (SmbiosStringAreaStart =3D=3D NULL)) {
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>> + if (SmbiosStringAreaSize < StringTableGetStringSetSize (StrTable)) {
>>> + return EFI_BUFFER_TOO_SMALL;
>>> + }
>>> +
>>> + SmbiosString =3D SmbiosStringAreaStart;
>>> + BytesRemaining =3D SmbiosStringAreaSize;
>>> +
>>> + if (StrTable->StrCount =3D=3D 0) {
>>> + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>>> + // If the formatted portion of the structure contains string-referenc=
e
>>> + // fields and all the string fields are set to 0 (no string reference=
s),
>>>=20
>>> + // the formatted section of the structure is followed by two null (00=
h)
>>> + // BYTES.
>>> + *SmbiosString++ =3D '\0';
>>> + } else {
>>> + for (Index =3D 0; Index < StrTable->StrCount; Index++) {
>>> + StrElement =3D &StrTable->Elements[Index];
>>> + AsciiStrCpyS (SmbiosString, BytesRemaining, StrElement->String);
>>> +
>>> + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>>> + // - Each string is terminated with a null (00h) BYTE
>>> + // Bytes Copied =3D String length + 1 for the string NULL terminator.
>>> + BytesCopied =3D StrElement->StringLen + 1;
>>> + BytesRemaining -=3D BytesCopied;
>>> + SmbiosString +=3D BytesCopied;
>>> + }
>>> + }
>>> +
>>> + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>>> + // - the set of strings is terminated with an additional null (00h)
>>> BYTE.
>>> + *SmbiosString =3D '\0';
>>=20
>> [GM] Shouldn't you advance the SmbiosString pointer by one more ? After
>> the loop isn't SmbiosString going to be at the NULL char of the last
>> string ? And we're trying to add one more NULL character ?
>> Should it be:
>> SmbiosString++;
>> *SmbiosString =3D '\0';
>=20
> I didn't try to run the code, but it seems ok to me. Assuming the string
> has 9 chars:
> """
> // Copy 9 chars + 1 NULL char
> AsciiStrCpyS (SmbiosString, BytesRemaining, StrElement->String);
>=20
> // We have copied 10 chars
> BytesCopied =3D StrElement->StringLen + 1;
> BytesRemaining -=3D BytesCopied;
> // Increment SmbiosString of 10 chars, so SmbiosString is pointing
> // to an un-initialized char now and we can continue.
> SmbiosString +=3D BytesCopied;
> """
>=20
> However, maybe we need to add an extra check/ASSERT for BytesRemaining
> before
> writing the last NULL char.

The check at the function entry i.e. "if (SmbiosStringAreaSize < StringTabl=
eGetStringSetSize (StrTable)) {"
should cover this, as BytesRemaining is derived from SmbiosStringAreaSize.
So as long as StringTableGetStringSetSize() returns the correct size, the a=
dditional check before writing
the last NULL char is not needed.

Please let me know if you think otherwise.

Regards,

Sami Mujawar

--h5K6MnLOoGCCBUjtH0Tr
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Pierre,<br /><br />On Tue, Oct 4, 2022 at 01:22 AM, PierreGondois wrote:=
<br />
<blockquote>
<blockquote>
<blockquote>+EFI_STATUS<br />+EFIAPI<br />+StringTablePublishStringSet (<br=
 />+ IN STRING_TABLE *CONST StrTable,<br />+ IN CHAR8 *CONST SmbiosStringAr=
eaStart,<br />+ IN CONST UINTN SmbiosStringAreaSize<br />+ )<br />+{<br />+=
 UINT8 Index;<br />+ STRING_ELEMENT *StrElement;<br />+ CHAR8 *SmbiosString=
;<br />+ UINTN BytesRemaining;<br />+ UINTN BytesCopied;<br />+<br />+ if (=
(StrTable =3D=3D NULL) || (SmbiosStringAreaStart =3D=3D NULL)) {<br />+ ret=
urn EFI_INVALID_PARAMETER;<br />+ }<br />+<br />+ if (SmbiosStringAreaSize =
&lt; StringTableGetStringSetSize (StrTable)) {<br />+ return EFI_BUFFER_TOO=
_SMALL;<br />+ }<br />+<br />+ SmbiosString =3D SmbiosStringAreaStart;<br /=
>+ BytesRemaining =3D SmbiosStringAreaSize;<br />+<br />+ if (StrTable-&gt;=
StrCount =3D=3D 0) {<br />+ // See Section 6.1.3 Text strings, SMBIOS Speci=
fication Version 3.6.0<br />+ // If the formatted portion of the structure =
contains string-reference<br />+ // fields and all the string fields are se=
t to 0 (no string references),<br />+ // the formatted section of the struc=
ture is followed by two null (00h)<br />+ // BYTES.<br />+ *SmbiosString++ =
=3D '\0';<br />+ } else {<br />+ for (Index =3D 0; Index &lt; StrTable-&gt;=
StrCount; Index++) {<br />+ StrElement =3D &amp;StrTable-&gt;Elements[Index=
];<br />+ AsciiStrCpyS (SmbiosString, BytesRemaining, StrElement-&gt;String=
);<br />+<br />+ // See Section 6.1.3 Text strings, SMBIOS Specification Ve=
rsion 3.6.0<br />+ // - Each string is terminated with a null (00h) BYTE<br=
 />+ // Bytes Copied =3D String length + 1 for the string NULL terminator.<=
br />+ BytesCopied =3D StrElement-&gt;StringLen + 1;<br />+ BytesRemaining =
-=3D BytesCopied;<br />+ SmbiosString +=3D BytesCopied;<br />+ }<br />+ }<b=
r />+<br />+ // See Section 6.1.3 Text strings, SMBIOS Specification Versio=
n 3.6.0<br />+ // - the set of strings is terminated with an additional nul=
l (00h) BYTE.<br />+ *SmbiosString =3D '\0';</blockquote>
[GM] Shouldn't you advance the SmbiosString pointer by one more ? After<br =
/>the loop isn't SmbiosString going to be at the NULL char of the last<br /=
>string ? And we're trying to add one more NULL character ?<br />Should it =
be:<br />SmbiosString++;<br />*SmbiosString =3D '\0';</blockquote>
I didn't try to run the code, but it seems ok to me. Assuming the string ha=
s 9 chars:<br />"""<br />// Copy 9 chars + 1 NULL char<br />AsciiStrCpyS (S=
mbiosString, BytesRemaining, StrElement-&gt;String);<br /><br />// We have =
copied 10 chars<br />BytesCopied =3D StrElement-&gt;StringLen + 1;<br />Byt=
esRemaining -=3D BytesCopied;<br />// Increment SmbiosString of 10 chars, s=
o SmbiosString is pointing<br />// to an un-initialized char now and we can=
 continue.<br />SmbiosString +=3D BytesCopied;<br />"""<br /><br />However,=
 maybe we need to add an extra check/ASSERT for BytesRemaining before<br />=
writing the last NULL char.</blockquote>
The check at the function entry i.e. "if (SmbiosStringAreaSize &lt; StringT=
ableGetStringSetSize (StrTable)) {" <br />should cover this, as BytesRemain=
ing is derived from SmbiosStringAreaSize.<br />So as long as StringTableGet=
StringSetSize() returns the correct size, the additional check before writi=
ng<br />the last NULL char is not needed.<br /><br />Please let me know if =
you think otherwise.<br /><br />Regards,<br /><br />Sami Mujawar

--h5K6MnLOoGCCBUjtH0Tr--