Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45459 )
Change subject: lib/spd: respect spd memory part name override if present ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/45459/12/src/lib/spd_bin.c@177 PS12, Line 177: spd_name[name_len + 1] = 0;
This is actually a change. e.g. […]
I had found the descrepancy while changing the code and considered adding a comment in code to point it out and explain my logic behind why I did not match the original code in this respect, but felt it probably better to point the issue out in a review comment instead of a large comment in the code.
I think it was incorrect / a minor bug to begin with, and here's why I think that.
The purpose of spd_get_name() is to get the "Module Part Number" field from the spd and return a null-terminated string containing that data in the input char * spd_name. In the actual SPD, from what I can tell from the JEDEC spec, the part number is not specified to be a null-terminated string, but is to have 0x20's anywhere that isn't the part name. In the case of this routine, the callers expect those trailing spaces to be stripped (or at a minimum expect the string to be null-terminated). Because a string can be the same size as DDR_SPD_PART_LEN, it must be stored into a buffer that is DDR_SPD_PART_LEN + 1 to assure space for the null termination, which the caller of this static routine does assure, so it is not an out of bounds access given the caller uses proper logic in picking the size of the spd_name buffer it passes. The logic of the previous code would chop off the last character of the part number in the string it returned IF the part number took up the entire field space with part name that contained DDR_SPD_PART_LEN significant characters (no null-termination and no 0x20 spaces). This version does not truncate that case.
This code is only ever used for logging purposes, so my guess is that this truncating behavior hasn't been noticed as one would have to encounter a part number that took up the entire space reserved in SPD AND an observant reviewer of the log to see that the last character in the "Module Part Number" field was truncated on that memory combination.
Would you agree, or am I missing something in my logic?