Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81548?usp=email )
Change subject: erasure_layout: do not copy region buffers if they are null ......................................................................
Patch Set 1:
(1 comment)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/81548/comment/84d5dcf4_1a0fb98d : PS1, Line 263: old_start_buf = (uint8_t *)malloc(old_start - region_start); : if (!old_start_buf) { : msg_cerr("Not enough memory!\n"); : ret = -1; : goto _end; : } : old_end_buf = (uint8_t *)malloc(region_end - old_end); : if (!old_end_buf) { : msg_cerr("Not enough memory!\n"); : ret = -1; : goto _end; : } Oh I think this piece is also somewhat relevant to the patch. I remember looking at this code with suspicion few times, but I always forgot to follow up. Thank you for finding time and cleaning that up!
The suspicion I had was that `old_start_buf` and `old_end_buf` are allocated for potentially 0 size (because `old_start - region_start` and `region_end - old_end` are checked later). And when they are 0 size they are not needed!
However allocating for 0 size, while not having any practical purpose, probably prevented `memcpy` below (at the `_end` label) from failing, because malloc linux lan page says
If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().
So I think this allocation should go under if conditions `if (old_start - region_start)` `if (region_end - old_end)` because otherwise there is nothing to allocate
Do you think this can go in the same this patch?