Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Fix Off by One Error When Creating X64 PlatformInfo.dat #317

Merged

Conversation

TaylorBeebe
Copy link
Contributor

@TaylorBeebe TaylorBeebe commented Sep 21, 2023

Description

AsciiSPrint() returns the string index non-inclusive of the NULL terminator, so adding 1 to the returned string index causes a NULL byte to be at the end of the PlatformInfo.dat file which can cause a parsing error when interpreted in .csv format in python.

  • Impacts functionality?
    • Functionality - Does the change ultimately impact how firmware functions?
    • Examples: Add a new library, publish a new PPI, update an algorithm, ...
  • Impacts security?
    • Security - Does the change have a direct security impact on an application,
      flow, or firmware?
    • Examples: Crypto algorithm change, buffer overflow fix, parameter
      validation improvement, ...
  • Breaking change?
    • Breaking change - Will anyone consuming this change experience a break
      in build or boot behavior?
    • Examples: Add a new library class, move a module to a different repo, call
      a function in a new library class in a pre-existing module, ...
  • Includes tests?
    • Tests - Does the change include any explicit test code?
    • Examples: Unit tests, integration tests, robot tests, ...
  • Includes documentation?
    • Documentation - Does the change contain explicit documentation additions
      outside direct code modifications (and comments)?
    • Examples: Update readme file, add feature readme file, link to documentation
      on an a separate Web page, ...

How This Was Tested

Tested on Q35

Integration Instructions

N/A

@makubacki
Copy link
Member

AsciiSPrint() returns the string index inclusive of the NULL terminator, so adding 1 to the returned string index causes an unknown byte at the end of the PlatformInfo.dat file.

The AsciiSPrint() return value states it includes the number of characters placed in the buffer not including the null-terminator. Are you saying you observed different behavior?

@TaylorBeebe
Copy link
Contributor Author

AsciiSPrint() returns the string index inclusive of the NULL terminator, so adding 1 to the returned string index causes an unknown byte at the end of the PlatformInfo.dat file.

The AsciiSPrint() return value states it includes the number of characters placed in the buffer not including the null-terminator. Are you saying you observed different behavior?

Incorrect description but correct change. The returned index does not include the null terminator and the null terminator shouldn't be included in the output as it causes a parsing error when interpreting the ascii bytes in csv format.

The other ASCII dat files also do not include the null terminator.

I'll update the description

@TaylorBeebe TaylorBeebe requested review from os-d and apop5 September 22, 2023 22:30
@TaylorBeebe TaylorBeebe changed the title Bugfix: Fix Off by One Error When Creating PlatformInfo.dat Bugfix: Fix Off by One Error When Creating X64 PlatformInfo.dat Sep 22, 2023
@makubacki
Copy link
Member

AsciiSPrint() returns the string index inclusive of the NULL terminator, so adding 1 to the returned string index causes an unknown byte at the end of the PlatformInfo.dat file.

The AsciiSPrint() return value states it includes the number of characters placed in the buffer not including the null-terminator. Are you saying you observed different behavior?

Incorrect description but correct change. The returned index does not include the null terminator and the null terminator shouldn't be included in the output as it causes a parsing error when interpreting the ascii bytes in csv format.

I'll update the description

Okay, that's what I suspected was intended based on the changes.

@TaylorBeebe TaylorBeebe enabled auto-merge (squash) September 26, 2023 15:54
@TaylorBeebe TaylorBeebe merged commit ac481a4 into microsoft:release/202302 Sep 26, 2023
30 checks passed
@makubacki makubacki added the type:bug Something isn't working label Sep 26, 2023
ProjectMuBot referenced this pull request in microsoft/mu_tiano_platforms Sep 27, 2023
Introduces 10 new commits in [Common/MU](https://github.com/microsoft/mu_plus.git).

<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/microsoft/mu_plus/commit/21b568873cc5de55e45245b2cb73fd3d7b9fe98f">21b568</a> GitHub Action: Bump actions/checkout from 3 to 4 (<a href="https://github.com/microsoft/mu_plus/pull/306">#306</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/39edcb7429889ff53531d1865f8905f3b43d29a9">39edcb</a> pip: bump antlr4-python3-runtime from 4.13.0 to 4.13.1 (<a href="https://github.com/microsoft/mu_plus/pull/308">#308</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/dffd6e30ebeb6f28d7733ae527287cdd8c0f42e7">dffd6e</a> pip: bump edk2-pytool-extensions from 0.24.0 to 0.24.1 (<a href="https://github.com/microsoft/mu_plus/pull/312">#312</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/a98b012c25fdbf6035699a1c78e3e217975edd2d">a98b01</a> Repo File Sync: Add check task to cargo makefile (<a href="https://github.com/microsoft/mu_plus/pull/313">#313</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/2d53120d882e2518f55eb2b4ccda67aeb6c91a81">2d5312</a> Repo File Sync: Update CodeQL GitHub workflow (<a href="https://github.com/microsoft/mu_plus/pull/314">#314</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/28c14f5b582f9e8d63856392436152a4c02efa0a">28c14f</a> pip: bump edk2-pytool-library from 0.17.0 to 0.18.0 (<a href="https://github.com/microsoft/mu_plus/pull/315">#315</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/dec8f57d3a5dc3f270f6b18940e99ba114d25f7a">dec8f5</a> Repo File Sync: Add cargo ecosystem to dependabot config (<a href="https://github.com/microsoft/mu_plus/pull/318">#318</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/506de6336ee721008bb23898a9747dce528a6875">506de6</a> Rust Dependency: Update spin requirement from 0.5.2 to 0.9.8 (<a href="https://github.com/microsoft/mu_plus/pull/319">#319</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/ac481a42ef21e79b9a5ecd6e82cea9fd2c1edaca">ac481a</a> Bugfix: Fix Off by One Error When Creating X64 PlatformInfo.dat (<a href="https://github.com/microsoft/mu_plus/pull/317">#317</a>)</li>
<li><a href="https://github.com/microsoft/mu_plus/commit/a76316c46525e60a08c7e794034979064289ee75">a76316</a> Repo File Sync: Update to Mu DevOps 6.5.1 (<a href="https://github.com/microsoft/mu_plus/pull/320">#320</a>)</li>
</ul>
</details>

Signed-off-by: Project Mu Bot <[email protected]>
@TaylorBeebe TaylorBeebe deleted the bugfix_platforminfodat branch November 10, 2023 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants