Attention is currently required from: Felix Singer, Paul Menzel, Angel Pons, Utkarsh Verma.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74128 )
Change subject: arch/x86/smbios: Avoid buffer overflows ......................................................................
Patch Set 7: Code-Review+1
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74128/comment/9d020c95_f675744d PS7, Line 7: arch/x86/smbios: Avoid buffer overflows "buffer overflow" usually refers to writing beyond the bounds of a buffer. This is not possible with `snprintf(buf, sizeof(buf), )`.
We could maybe say "string overflow"? In any case, it's hard to explain why we do this if there is nothing to fix, so how about "arch/x86/smbios: Refactor snprintf() uses" for the commit summary and then explain what and why in the commit message body.
https://review.coreboot.org/c/coreboot/+/74128/comment/4b3281bc_dd4c3e45 PS7, Line 11: static analysers. This commit also alters buffer sizes, please mention that as well.
https://review.coreboot.org/c/coreboot/+/74128/comment/c531c12c_fe69611c PS7, Line 14: https://scan6.scan.coverity.com/reports.htm#v55284/p10744 If you happen to have a link to the coreboot mailing list archive (Coverity is supposed to report all issues on the ML), that would be better. Links that require a login are discouraged and there's also no guarantee that this link will be still valid after 1, 2, 3 months. The commit message, OTOH, is forever.
Patchset:
PS7: Commit message needs more work, looks good to me otherwise.
The commit message is quite important already during review. If you cover everything in the commit message, you can avoid bikeshedding (some at least) and confusion.
File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/74128/comment/c6b5a7ab_48a16f66 PS4, Line 148: char string_buffer[15];
Wait, I just checked it again. Shouldn't 15 buffer size be enough? […]
Yes, you are right :) I must have missed that it's hex.
-
Nico Huber (Code Review)