Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40028
to review the following change.
Change subject: libpayload: malloc: Change memcpy() to memmove() in realloc ......................................................................
libpayload: malloc: Change memcpy() to memmove() in realloc
Our realloc() works (somewhat suboptimally) by free()ing the existing allocation and then reallocating it wherever it fits. If there was free space before the old location, this means the new allocation may be before the old one, and if the free space block is smaller than the old allocation it may overlap. Thus, we should be moving memmove() instead of memcpy() to move the block over.
This is not a problem in practice since all our existing memcpy()s are simple iterate and copy front to back implementations which are safe for overlaps when the destination is in front of the source. but it's still the more correct thing to do (in case we ever change our memcpy()s to do something more advanced or whatever).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I35f77a94b7a72c01364ee7eecb5c3ff5ecde57f6 --- M payloads/libpayload/libc/malloc.c 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/40028/1
diff --git a/payloads/libpayload/libc/malloc.c b/payloads/libpayload/libc/malloc.c index 1fdb59e..f2a54a7 100644 --- a/payloads/libpayload/libc/malloc.c +++ b/payloads/libpayload/libc/malloc.c @@ -310,8 +310,9 @@ if (ret == NULL || ret == ptr) return ret;
- /* Copy the memory to the new location. */ - memcpy(ret, ptr, osize > size ? size : osize); + /* Move the memory to the new location. Might be before the old location + and overlap since the free() above includes a _consolidate(). */ + memmove(ret, ptr, osize > size ? size : osize);
return ret; }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40028 )
Change subject: libpayload: malloc: Change memcpy() to memmove() in realloc ......................................................................
Patch Set 1: Code-Review+2
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40028 )
Change subject: libpayload: malloc: Change memcpy() to memmove() in realloc ......................................................................
libpayload: malloc: Change memcpy() to memmove() in realloc
Our realloc() works (somewhat suboptimally) by free()ing the existing allocation and then reallocating it wherever it fits. If there was free space before the old location, this means the new allocation may be before the old one, and if the free space block is smaller than the old allocation it may overlap. Thus, we should be moving memmove() instead of memcpy() to move the block over.
This is not a problem in practice since all our existing memcpy()s are simple iterate and copy front to back implementations which are safe for overlaps when the destination is in front of the source. but it's still the more correct thing to do (in case we ever change our memcpy()s to do something more advanced or whatever).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I35f77a94b7a72c01364ee7eecb5c3ff5ecde57f6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40028 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M payloads/libpayload/libc/malloc.c 1 file changed, 3 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/payloads/libpayload/libc/malloc.c b/payloads/libpayload/libc/malloc.c index 1fdb59e..f2a54a7 100644 --- a/payloads/libpayload/libc/malloc.c +++ b/payloads/libpayload/libc/malloc.c @@ -310,8 +310,9 @@ if (ret == NULL || ret == ptr) return ret;
- /* Copy the memory to the new location. */ - memcpy(ret, ptr, osize > size ? size : osize); + /* Move the memory to the new location. Might be before the old location + and overlap since the free() above includes a _consolidate(). */ + memmove(ret, ptr, osize > size ? size : osize);
return ret; }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40028 )
Change subject: libpayload: malloc: Change memcpy() to memmove() in realloc ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2030 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2029 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2028
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Julius Werner (Code Review)