May 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Anastasia Klimchuk (Code Review) 31 May '21

31 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Nico Huber, Namyoon Woo, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/54748 to look at the new patch set (#4). Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... dummyflasher.c: Extract params processing into a separate function This makes init function easier to read. A good side-effect of the change is: 13 error paths of params processing are not leaking data anymore. All those error paths return 1 back to init function which frees data. And there was just one more error path in init function were free(data) needed to be added. BUG=b:185191942 TEST=ninja test Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 51 insertions(+), 27 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/54748/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Namyoon Woo <namyoon(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Edward O'Callaghan (Code Review) 31 May '21

31 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 ) Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... Patch Set 3: (1 comment) File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/54748/comment/abe2e9d0_37adf177 PS3, Line 993: = BUS_NONE; ; init_data() already initialises the value -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Namyoon Woo <namyoon(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 31 May 2021 00:12:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Anastasia Klimchuk (Code Review) 31 May '21

31 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54748 ) Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... Patch Set 3: (4 comments) Patchset: PS3: Rebased on the top of all latest improvements and fixes. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/54748/comment/5c7a8a8b_6e4db04e PS1, Line 26: /* Remove the #define below if you don't want SPI flash chip emulation. */ : #define EMULATE_SPI_CHIP 1 > If this line is removed, dummy does not compile. […] This comment still valid, I think. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/54748/comment/962737ad_d17c4b46 PS2, Line 975: return 1; > This will result in `data->flashchip_contents` leaking. […] I replied below https://review.coreboot.org/c/flashrom/+/54748/comment/c34221e2_d1f975c6 PS2, Line 1014: > I moved around the initialisation in CB:54908 I moved initialisation in the same way (rebased), but I had a choice where to put it: init_data vs dummy_init. To me it looks like it can be in init_data? (because it inits data). And all the memory leaks seems to be fixed by now... -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Namyoon Woo <namyoon(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 May 2021 23:58:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher.c: Extract params processing into a separate function
by Anastasia Klimchuk (Code Review) 31 May '21

31 May '21

Attention is currently required from: Nico Huber, Namyoon Woo, Edward O'Callaghan, Angel Pons. Hello build bot (Jenkins), Nico Huber, Namyoon Woo, Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/54748 to look at the new patch set (#3). Change subject: dummyflasher.c: Extract params processing into a separate function ...................................................................... dummyflasher.c: Extract params processing into a separate function This makes init function easier to read. A good side-effect of the change is: 13 error paths of params processing are not leaking data anymore. All those error paths return 1 back to init function which frees data. And there was just one more error path in init function were free(data) needed to be added. BUG=b:185191942 TEST=ninja test Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 51 insertions(+), 27 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/48/54748/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/54748 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04f55f77bb4703f1d88b2191c45a22be3c97bf87 Gerrit-Change-Number: 54748 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Namyoon Woo <namyoon(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: layout: Introduce flashrom_layout_add_region()
by Edward O'Callaghan (Code Review) 30 May '21

30 May '21

Attention is currently required from: Nico Huber, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33517 ) Change subject: layout: Introduce flashrom_layout_add_region() ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/33517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I50d473d0d5d1fb38bd6f9ae3d7127e9ea66a94e1 Gerrit-Change-Number: 33517 Gerrit-PatchSet: 6 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Sun, 30 May 2021 02:11:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: bitbang_spi.c: Pass spi data into bitbang_spi_master functions
by Edward O'Callaghan (Code Review) 29 May '21

29 May '21

Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54992 ) Change subject: bitbang_spi.c: Pass spi data into bitbang_spi_master functions ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54992 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I90a340aeb9ed70605f5963aa66ee913d22bff865 Gerrit-Change-Number: 54992 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 29 May 2021 01:22:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: developerbox_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 29 May '21

29 May '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54993 ) Change subject: developerbox_spi.c: Refactor singleton states into reentrant pattern ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/54993 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I84bf0cffa4a14e58103bdb1e7e65ec2475344f12 Gerrit-Change-Number: 54993 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sat, 29 May 2021 01:12:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher.c: Fix typo in log message
by Edward O'Callaghan (Code Review) 28 May '21

28 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54910 ) Change subject: dummyflasher.c: Fix typo in log message ...................................................................... dummyflasher.c: Fix typo in log message multipler ---> multiple Change-Id: I16fbc0072c82ac152af5f1d65df196cdd113e03b Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54910 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/dummyflasher.c b/dummyflasher.c index 560dbdc..dcf97b5 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -831,7 +831,7 @@ if (tmp) { size = strtol(tmp, NULL, 10); if (size <= 0 || (size % 1024 != 0)) { - msg_perr("%s: Chip size is not a multipler of 1024: %s\n", + msg_perr("%s: Chip size is not a multiple of 1024: %s\n", __func__, tmp); free(tmp); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/54910 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I16fbc0072c82ac152af5f1d65df196cdd113e03b Gerrit-Change-Number: 54910 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dummyflasher.c: Prevent use-after-free bug
by Edward O'Callaghan (Code Review) 28 May '21

28 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54909 ) Change subject: dummyflasher.c: Prevent use-after-free bug ...................................................................... dummyflasher.c: Prevent use-after-free bug The memory for the `status` string is aliased by the `endptr` pointer. Moreover, `errno` could have been modified by the call to `free()`. Therefore, only free the former when there are no more uses of either. Change-Id: I1b56834004fe18918213a7df0a09a8a7ecb56985 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54909 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 2 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/dummyflasher.c b/dummyflasher.c index 5defec0..560dbdc 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -962,12 +962,13 @@ if (status) { errno = 0; data->emu_status = strtoul(status, &endptr, 0); - free(status); if (errno != 0 || status == endptr) { + free(status); msg_perr("Error: initial status register specified, " "but the value could not be converted.\n"); return 1; } + free(status); msg_pdbg("Initial status register is set to 0x%02x.\n", data->emu_status); } -- To view, visit https://review.coreboot.org/c/flashrom/+/54909 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I1b56834004fe18918213a7df0a09a8a7ecb56985 Gerrit-Change-Number: 54909 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dummyflasher.c: Move `flashchip_contents` allocation
by Edward O'Callaghan (Code Review) 28 May '21

28 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54908 ) Change subject: dummyflasher.c: Move `flashchip_contents` allocation ...................................................................... dummyflasher.c: Move `flashchip_contents` allocation Place `flashchip_contents` allocation next to the code that initialises the newly-allocated buffer. This also avoids leaking it if the handling of `spi_status` fails. Change-Id: Ie907ac88dfe4ca018c97d9ce6ce042b4ffacf36a Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54908 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 6 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/dummyflasher.c b/dummyflasher.c index 717f8d1..5defec0 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -957,12 +957,6 @@ } free(tmp); - data->flashchip_contents = malloc(data->emu_chip_size); - if (!data->flashchip_contents) { - msg_perr("Out of memory!\n"); - return 1; - } - #ifdef EMULATE_SPI_CHIP status = extract_programmer_param("spi_status"); if (status) { @@ -979,6 +973,12 @@ } #endif + data->flashchip_contents = malloc(data->emu_chip_size); + if (!data->flashchip_contents) { + msg_perr("Out of memory!\n"); + return 1; + } + msg_pdbg("Filling fake flash chip with 0x%02x, size %i\n", data->erase_to_zero ? 0x00 : 0xff, data->emu_chip_size); memset(data->flashchip_contents, data->erase_to_zero ? 0x00 : 0xff, data->emu_chip_size); -- To view, visit https://review.coreboot.org/c/flashrom/+/54908 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie907ac88dfe4ca018c97d9ce6ce042b4ffacf36a Gerrit-Change-Number: 54908 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021