Name of user not set #1003143 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
commonlib/region: Add null parameters handling
Prevent NULL-pointer dereference of child or parent root_device parameter.
Signed-off-by: Jakub Czapiga jacz@semihalf.com Change-Id: I6b9c2b4a3dc2f4f31a40fd045d1fa2d2d1df3eca --- M src/commonlib/region.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/48627/1
diff --git a/src/commonlib/region.c b/src/commonlib/region.c index a10702a..efb7b1f 100644 --- a/src/commonlib/region.c +++ b/src/commonlib/region.c @@ -141,6 +141,9 @@ .size = size, };
+ if (child == NULL || parent == NULL) + return -1; + if (!normalize_and_ok(&parent->region, &req)) return -1;
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Patch Set 1: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG@9 PS1, Line 9: Prevent NULL-pointer dereference of child or parent root_device parameter. I see you're adding multiple of these recently, but is there a particular reason for it in this case? I don't think we subscribe to the philosophy that every pointer argument ever should always be checked for NULL (that would just seem like pointless binary bloat). If it's a function where passing NULL seems like a likely mistake or there's a risk of it taking untrusted data adding checks would be fine, but with something simple and straight-forward like this I don't really see the point.
Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG@9 PS1, Line 9: Prevent NULL-pointer dereference of child or parent root_device parameter.
I see you're adding multiple of these recently, but is there a particular reason for it in this case […]
After reviewing this patch I see your point. There is no need to add this check. I did it, because lib/fmap tests were failing after passing NULL as pointer to output structure. Would assert be better here?
With no NULL-check I will have to change lib/fmap tests and exclude this cases. I was thinking about leaving them as a comment with description.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG@9 PS1, Line 9: Prevent NULL-pointer dereference of child or parent root_device parameter.
After reviewing this patch I see your point. There is no need to add this check. […]
Yes, if you want a check an assert() would probably be more appropriate. But I also don't think this behavior is super important to test for, a null pointer access is usually pretty easy to notice. I would rather have the tests focus more thoroughly on the normal behavior of these functions.
Name of user not set #1003143 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48627/1//COMMIT_MSG@9 PS1, Line 9: Prevent NULL-pointer dereference of child or parent root_device parameter.
Yes, if you want a check an assert() would probably be more appropriate. […]
Thank You. So, I will abandon this change, as it is not needed.
Name of user not set #1003143 has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48627 )
Change subject: commonlib/region: Add null parameters handling ......................................................................
Abandoned
Change adds unnecessary code.
-
Julius Werner (Code Review) -
Name of user not set (Code Review)
-
Paul Fagerburg (Code Review)