Hello Sami, On 3/9/23 12:11, Sami Mujawar wrote:
Hi Pierre,On Tue, Oct 4, 2022 at 01:22 AM, PierreGondois wrote: +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 == NULL) || (SmbiosStringAreaStart == NULL)) { + return EFI_INVALID_PARAMETER; + } + + if (SmbiosStringAreaSize < StringTableGetStringSetSize (StrTable)) { + return EFI_BUFFER_TOO_SMALL; + } + + SmbiosString = SmbiosStringAreaStart; + BytesRemaining = SmbiosStringAreaSize; + + if (StrTable->StrCount == 0) { + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0 + // If the formatted portion of the structure contains string-reference + // fields and all the string fields are set to 0 (no string references), + // the formatted section of the structure is followed by two null (00h) + // BYTES. + *SmbiosString++ = '\0'; + } else { + for (Index = 0; Index < StrTable->StrCount; Index++) { + StrElement = &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 = String length + 1 for the string NULL terminator. + BytesCopied = StrElement->StringLen + 1; + BytesRemaining -= BytesCopied; + SmbiosString += 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 = '\0'; [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 = '\0'; 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); // We have copied 10 chars BytesCopied = StrElement->StringLen + 1; BytesRemaining -= BytesCopied; // Increment SmbiosString of 10 chars, so SmbiosString is pointing // to an un-initialized char now and we can continue. SmbiosString += BytesCopied; """ 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 < StringTableGetStringSetSize (StrTable)) {" should cover this, as BytesRemaining is derived from SmbiosStringAreaSize. So as long as StringTableGetStringSetSize() returns the correct size, the additional check before writing the last NULL char is not needed.
Ok sure, it was just a suggestion. Also just in case, there was this remark from you: https://edk2.groups.io/g/devel/message/93654 Otherwise: Reviewed-by: Pierre Gondois <pierre.gond...@arm.com> Regards, Pierre
Please let me know if you think otherwise. Regards, Sami Mujawar
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100990): https://edk2.groups.io/g/devel/message/100990 Mute This Topic: https://groups.io/mt/93633189/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
