Thejaswani Putta has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32290
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/coreboot_table.c 1 file changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/1
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 6e44f5d..808cfa7 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -233,8 +233,11 @@ vbwb = (struct lb_range *)lb_new_record(header); vbwb->tag = LB_TAB_VBOOT_WORKBUF; vbwb->size = sizeof(*vbwb); - vbwb->range_start = (uintptr_t)wd + wd->buffer_offset; - vbwb->range_size = wd->buffer_size; + if(wd) + { + vbwb->range_start = (uintptr_t)wd + wd->buffer_offset; + vbwb->range_size = wd->buffer_size; + } }
__weak uint32_t board_id(void) { return UNDEFINED_STRAPPING_ID; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c@236 PS1, Line 236: if(wd) code indent should use tabs where possible
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c@236 PS1, Line 236: if(wd) please, no spaces at the start of a line
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c@236 PS1, Line 236: if(wd) that open brace { should be on the previous line
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c@236 PS1, Line 236: if(wd) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/1/src/lib/coreboot_table.c@237 PS1, Line 237: { trailing whitespace
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#2).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/coreboot_table.c M src/lib/string.c M src/security/vboot/common.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 5 files changed, 22 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 2:
(15 comments)
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c@236 PS2, Line 236: if(wd) code indent should use tabs where possible
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c@236 PS2, Line 236: if(wd) please, no spaces at the start of a line
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c@236 PS2, Line 236: if(wd) that open brace { should be on the previous line
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c@236 PS2, Line 236: if(wd) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/2/src/lib/coreboot_table.c@237 PS2, Line 237: { trailing whitespace
https://review.coreboot.org/#/c/32290/2/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@9 PS2, Line 9: if(d) trailing whitespace
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@9 PS2, Line 9: if(d) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@19 PS2, Line 19: if(d) trailing whitespace
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@19 PS2, Line 19: if(d) code indent should use tabs where possible
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@19 PS2, Line 19: if(d) please, no spaces at the start of a line
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@19 PS2, Line 19: if(d) that open brace { should be on the previous line
https://review.coreboot.org/#/c/32290/2/src/lib/string.c@19 PS2, Line 19: if(d) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/2/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/32290/2/src/security/vboot/common.c@79 PS2, Line 79: if(wd) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/2/src/soc/intel/broadwell/ramstage.c File src/soc/intel/broadwell/ramstage.c:
https://review.coreboot.org/#/c/32290/2/src/soc/intel/broadwell/ramstage.c@3... PS2, Line 34: if(!ps) space required before the open parenthesis '('
https://review.coreboot.org/#/c/32290/2/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/2/util/cbfstool/cbfs_sections.c@68 PS2, Line 68: if(!list_node) space required before the open parenthesis '('
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#3).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/coreboot_table.c M src/lib/string.c M src/security/vboot/common.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 5 files changed, 22 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@236 PS3, Line 236: if (wd) that open brace { should be on the previous line
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@237 PS3, Line 237: { trailing whitespace
https://review.coreboot.org/#/c/32290/3/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32290/3/src/lib/string.c@19 PS3, Line 19: if (d) that open brace { should be on the previous line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@236 PS3, Line 236: if (wd) Are you running your static analysis with CONFIG_FATAL_ASSERTS? Because vboot_get_working_data() cannot return NULL -- there's literally an assert(wd != NULL) right at the end.
https://review.coreboot.org/#/c/32290/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/32290/3/src/security/vboot/common.c@79 PS3, Line 79: if (wd) Same applies here, wd cannot be NULL.
https://review.coreboot.org/#/c/32290/3/src/soc/intel/broadwell/ramstage.c File src/soc/intel/broadwell/ramstage.c:
https://review.coreboot.org/#/c/32290/3/src/soc/intel/broadwell/ramstage.c@3... PS3, Line 35: return 0; I don't know this code, but if this was written in a way that always expected CBMEM_ID_POWER_STATE to be found here, the correct fix is probably to add an assert() rather than to silently return if it's missing.
https://review.coreboot.org/#/c/32290/3/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/3/util/cbfstool/cbfs_sections.c@68 PS3, Line 68: if (!list_node) You should print an ERROR() here before you return.
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#4).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/string.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 3 files changed, 13 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/32290/4/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32290/4/src/lib/string.c@9 PS4, Line 9: if (d) { braces {} are not necessary for single statement blocks
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c@69 PS4, Line 69: printf("Error, list_node pointer checked for NULL failed! \n"); unnecessary whitespace before a quoted newline
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#5).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/string.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 3 files changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c@69 PS4, Line 69: printf("Error, list_node pointer checked for NULL failed! \n");
unnecessary whitespace before a quoted newline
Can't you use the ERROR() macro here?
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#6).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/string.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 3 files changed, 13 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/6
Thejaswani Putta has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 6:
(7 comments)
Patch Set 5:
(1 comment)
Done, Thanks!
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@236 PS3, Line 236: if (wd)
Are you running your static analysis with CONFIG_FATAL_ASSERTS? Because vboot_get_working_data() can […]
Done
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@236 PS3, Line 236: if (wd)
that open brace { should be on the previous line
Done
https://review.coreboot.org/#/c/32290/3/src/lib/coreboot_table.c@237 PS3, Line 237: {
trailing whitespace
Done
https://review.coreboot.org/#/c/32290/4/src/lib/string.c File src/lib/string.c:
https://review.coreboot.org/#/c/32290/4/src/lib/string.c@9 PS4, Line 9: if (d) {
braces {} are not necessary for single statement blocks
Ack
https://review.coreboot.org/#/c/32290/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/32290/3/src/security/vboot/common.c@79 PS3, Line 79: if (wd)
Same applies here, wd cannot be NULL.
Ack
https://review.coreboot.org/#/c/32290/3/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/3/util/cbfstool/cbfs_sections.c@68 PS3, Line 68: if (!list_node)
You should print an ERROR() here before you return.
Ack
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/4/util/cbfstool/cbfs_sections.c@69 PS4, Line 69: printf("Error, list_node pointer checked for NULL failed! \n");
unnecessary whitespace before a quoted newline
Ack
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32290/6/src/soc/intel/broadwell/ramstage.c File src/soc/intel/broadwell/ramstage.c:
https://review.coreboot.org/#/c/32290/6/src/soc/intel/broadwell/ramstage.c@3... PS6, Line 34: assert(ps != NULL); needs <assert.h>
https://review.coreboot.org/#/c/32290/6/util/cbfstool/cbfs_sections.c File util/cbfstool/cbfs_sections.c:
https://review.coreboot.org/#/c/32290/6/util/cbfstool/cbfs_sections.c@70 PS6, Line 70: list_node pointer checked for NULL failed nit: that's a little too low-level, maybe... how about "cannot allocate CBFS flag node"?
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32290
to look at the new patch set (#7).
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 --- M src/lib/string.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 3 files changed, 14 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/32290/7
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 7: Code-Review+2
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32290 )
Change subject: Klocwork: Fix the Null pointer derefernce found by klocwork ......................................................................
Klocwork: Fix the Null pointer derefernce found by klocwork
Signed-off-by: Thejaswani Putta thejaswani.putta@intel.com Change-Id: I15973ac28e9645826986cf63d2160eedb83024e4 Reviewed-on: https://review.coreboot.org/c/coreboot/+/32290 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Lijian Zhao lijian.zhao@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/string.c M src/soc/intel/broadwell/ramstage.c M util/cbfstool/cbfs_sections.c 3 files changed, 14 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Lijian Zhao: Looks good to me, approved
diff --git a/src/lib/string.c b/src/lib/string.c index df2fd80..2e71489f 100644 --- a/src/lib/string.c +++ b/src/lib/string.c @@ -6,7 +6,8 @@ { size_t sz = strlen(s) + 1; char *d = malloc(sz); - memcpy(d, s, sz); + if (d) + memcpy(d, s, sz); return d; }
@@ -15,7 +16,9 @@ size_t sz_1 = strlen(s1); size_t sz_2 = strlen(s2); char *d = malloc(sz_1 + sz_2 + 1); - memcpy(d, s1, sz_1); - memcpy(d + sz_1, s2, sz_2 + 1); + if (d) { + memcpy(d, s1, sz_1); + memcpy(d + sz_1, s2, sz_2 + 1); + } return d; } diff --git a/src/soc/intel/broadwell/ramstage.c b/src/soc/intel/broadwell/ramstage.c index e1883f2..7065369 100644 --- a/src/soc/intel/broadwell/ramstage.c +++ b/src/soc/intel/broadwell/ramstage.c @@ -23,6 +23,7 @@ #include <soc/ramstage.h> #include <soc/intel/broadwell/chip.h> #include <soc/intel/common/acpi.h> +#include <assert.h>
/* Save wake source information for calculating ACPI _SWS values */ int soc_fill_acpi_wake(uint32_t *pm1, uint32_t **gpe0) @@ -31,6 +32,8 @@ static uint32_t gpe0_sts[GPE0_REG_MAX]; int i;
+ assert(ps != NULL); + *pm1 = ps->pm1_sts & ps->pm1_en;
/* Mask off GPE0 status bits that are not enabled */ diff --git a/util/cbfstool/cbfs_sections.c b/util/cbfstool/cbfs_sections.c index 2857257..088534a 100644 --- a/util/cbfstool/cbfs_sections.c +++ b/util/cbfstool/cbfs_sections.c @@ -14,6 +14,7 @@ */
#include "cbfs_sections.h" +#include "common.h"
#include <assert.h> #include <stdlib.h> @@ -65,6 +66,10 @@ return false;
list_node = (struct descriptor_node *)malloc(sizeof(*list_node)); + if (!list_node) { + ERROR("Cannot allocate CBFS flag node!\n"); + return false; + } list_node->val = node; list_node->next = NULL;