| Message ID | 1398411891-26363-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On 25/04/14 08:44, Ivan Hu wrote: > While using a simple hash bucket algorithm to check the uniqueness of each > variable and error if we encounter any duplicates, it used the same memory > address for GUID checking in each buckets. It makes the GUID would be identical, > so the result will be unexpected. After I figured out what the code was doing I realised what the final sentence should probably be: "Unfortunately, this makes the GUIDs identical, which causes incorrect GUID matches." Apart from that, it's good to me. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 4b32693..5d31ad8 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -520,6 +520,7 @@ static void bucket_destroy(void) > struct efi_var_item *chain = item->next; > > free(item->name); > + free(item->guid); > free(item); > item = chain; > } > @@ -574,14 +575,24 @@ static int getnextvariable_test3(fwts_framework *fw) > goto err; > } > > - item->guid = &vendorguid; > - item->next = NULL; > + item->guid = malloc(sizeof(EFI_GUID)); > + if (!item->guid) { > + fwts_failed(fw, LOG_LEVEL_HIGH, > + "UEFIRuntimeGetNextVariableName", > + "Failed to allocate memory for test."); > + free(item); > + goto err; > + } > > + memcpy(item->guid, &vendorguid, sizeof(EFI_GUID)); > + > + item->next = NULL; > item->name = malloc(variablenamesize); > if (!item->name) { > fwts_failed(fw, LOG_LEVEL_HIGH, > "UEFIRuntimeGetNextVariableName", > "Failed to allocate memory for test."); > + free(item->guid); > free(item); > goto err; > } > @@ -596,6 +607,7 @@ static int getnextvariable_test3(fwts_framework *fw) > "UEFIRuntimeGetNextVariableName", > "Duplicate variable name found."); > free(item->name); > + free(item->guid); > free(item); > goto err; > } > Acked-by: Colin Ian King <colin.king@canonical.com>
On 04/25/2014 04:27 PM, Colin Ian King wrote: > On 25/04/14 08:44, Ivan Hu wrote: >> While using a simple hash bucket algorithm to check the uniqueness of each >> variable and error if we encounter any duplicates, it used the same memory >> address for GUID checking in each buckets. It makes the GUID would be identical, >> so the result will be unexpected. > > After I figured out what the code was doing I realised what the final > sentence should probably be: > > "Unfortunately, this makes the GUIDs identical, which causes incorrect > GUID matches." > > Apart from that, it's good to me. Thanks Colin, my poor English.:) I'll amend the sentence when I push the patch to upstream. > > >> >> Signed-off-by: Ivan Hu <ivan.hu@canonical.com> >> --- >> src/uefi/uefirtvariable/uefirtvariable.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c >> index 4b32693..5d31ad8 100644 >> --- a/src/uefi/uefirtvariable/uefirtvariable.c >> +++ b/src/uefi/uefirtvariable/uefirtvariable.c >> @@ -520,6 +520,7 @@ static void bucket_destroy(void) >> struct efi_var_item *chain = item->next; >> >> free(item->name); >> + free(item->guid); >> free(item); >> item = chain; >> } >> @@ -574,14 +575,24 @@ static int getnextvariable_test3(fwts_framework *fw) >> goto err; >> } >> >> - item->guid = &vendorguid; >> - item->next = NULL; >> + item->guid = malloc(sizeof(EFI_GUID)); >> + if (!item->guid) { >> + fwts_failed(fw, LOG_LEVEL_HIGH, >> + "UEFIRuntimeGetNextVariableName", >> + "Failed to allocate memory for test."); >> + free(item); >> + goto err; >> + } >> >> + memcpy(item->guid, &vendorguid, sizeof(EFI_GUID)); >> + >> + item->next = NULL; >> item->name = malloc(variablenamesize); >> if (!item->name) { >> fwts_failed(fw, LOG_LEVEL_HIGH, >> "UEFIRuntimeGetNextVariableName", >> "Failed to allocate memory for test."); >> + free(item->guid); >> free(item); >> goto err; >> } >> @@ -596,6 +607,7 @@ static int getnextvariable_test3(fwts_framework *fw) >> "UEFIRuntimeGetNextVariableName", >> "Duplicate variable name found."); >> free(item->name); >> + free(item->guid); >> free(item); >> goto err; >> } >> > > > Acked-by: Colin Ian King <colin.king@canonical.com> >
On 04/25/2014 03:44 PM, Ivan Hu wrote: > While using a simple hash bucket algorithm to check the uniqueness of each > variable and error if we encounter any duplicates, it used the same memory > address for GUID checking in each buckets. It makes the GUID would be identical, > so the result will be unexpected. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/uefi/uefirtvariable/uefirtvariable.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c > index 4b32693..5d31ad8 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -520,6 +520,7 @@ static void bucket_destroy(void) > struct efi_var_item *chain = item->next; > > free(item->name); > + free(item->guid); > free(item); > item = chain; > } > @@ -574,14 +575,24 @@ static int getnextvariable_test3(fwts_framework *fw) > goto err; > } > > - item->guid = &vendorguid; > - item->next = NULL; > + item->guid = malloc(sizeof(EFI_GUID)); > + if (!item->guid) { > + fwts_failed(fw, LOG_LEVEL_HIGH, > + "UEFIRuntimeGetNextVariableName", > + "Failed to allocate memory for test."); > + free(item); > + goto err; > + } > > + memcpy(item->guid, &vendorguid, sizeof(EFI_GUID)); > + > + item->next = NULL; > item->name = malloc(variablenamesize); > if (!item->name) { > fwts_failed(fw, LOG_LEVEL_HIGH, > "UEFIRuntimeGetNextVariableName", > "Failed to allocate memory for test."); > + free(item->guid); > free(item); > goto err; > } > @@ -596,6 +607,7 @@ static int getnextvariable_test3(fwts_framework *fw) > "UEFIRuntimeGetNextVariableName", > "Duplicate variable name found."); > free(item->name); > + free(item->guid); > free(item); > goto err; > } > Acked-by: Alex Hung <alex.hung@canonical.com>
diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index 4b32693..5d31ad8 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -520,6 +520,7 @@ static void bucket_destroy(void) struct efi_var_item *chain = item->next; free(item->name); + free(item->guid); free(item); item = chain; } @@ -574,14 +575,24 @@ static int getnextvariable_test3(fwts_framework *fw) goto err; } - item->guid = &vendorguid; - item->next = NULL; + item->guid = malloc(sizeof(EFI_GUID)); + if (!item->guid) { + fwts_failed(fw, LOG_LEVEL_HIGH, + "UEFIRuntimeGetNextVariableName", + "Failed to allocate memory for test."); + free(item); + goto err; + } + memcpy(item->guid, &vendorguid, sizeof(EFI_GUID)); + + item->next = NULL; item->name = malloc(variablenamesize); if (!item->name) { fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName", "Failed to allocate memory for test."); + free(item->guid); free(item); goto err; } @@ -596,6 +607,7 @@ static int getnextvariable_test3(fwts_framework *fw) "UEFIRuntimeGetNextVariableName", "Duplicate variable name found."); free(item->name); + free(item->guid); free(item); goto err; }
While using a simple hash bucket algorithm to check the uniqueness of each variable and error if we encounter any duplicates, it used the same memory address for GUID checking in each buckets. It makes the GUID would be identical, so the result will be unexpected. Signed-off-by: Ivan Hu <ivan.hu@canonical.com> --- src/uefi/uefirtvariable/uefirtvariable.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)