| Message ID | 1458519472-10914-1-git-send-email-cyril.bur@au1.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
On 21/03/16 11:17, Cyril Bur wrote: > The arch_flash_init() in arch_flash_x86.c doesn't actually check the return value > of file_init_path(), rather it is comparing the returned structure against NULL. > It is unsafe (and incorrect at the moment) to assume that file_init_path will > NULL this value on failure, it doesn't have to as it returns a value to > indicate success or failure. > > The arch_flash_init() in arch_flash_powerpc.c calls file_init_path() through > another function which will return a pointer (or NULL on failure), this > function doesn't explicitly NULL its return pointer in the case that > file_init_path() fails. It has initialised the pointer to NULL so the case may > be less severe (compared to the arch_flash_x86 problem) as file_init_path() > shouldn't have changed it on failure case, however, assuming that it won't > is unsafe. It is best to explicitly NULL the return pointer if file_init_path() > returns a failure. > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> Looks good! Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Cyril Bur <cyril.bur@au1.ibm.com> writes: > The arch_flash_init() in arch_flash_x86.c doesn't actually check the return value > of file_init_path(), rather it is comparing the returned structure against NULL. > It is unsafe (and incorrect at the moment) to assume that file_init_path will > NULL this value on failure, it doesn't have to as it returns a value to > indicate success or failure. > > The arch_flash_init() in arch_flash_powerpc.c calls file_init_path() through > another function which will return a pointer (or NULL on failure), this > function doesn't explicitly NULL its return pointer in the case that > file_init_path() fails. It has initialised the pointer to NULL so the case may > be less severe (compared to the arch_flash_x86 problem) as file_init_path() > shouldn't have changed it on failure case, however, assuming that it won't > is unsafe. It is best to explicitly NULL the return pointer if file_init_path() > returns a failure. > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > --- > external/common/arch_flash_powerpc.c | 4 +++- > external/common/arch_flash_x86.c | 5 +++-- > 2 files changed, 6 insertions(+), 3 deletions(-) Thanks, merged as of 145312a
diff --git a/external/common/arch_flash_powerpc.c b/external/common/arch_flash_powerpc.c index 19dfec8..7ce962e 100644 --- a/external/common/arch_flash_powerpc.c +++ b/external/common/arch_flash_powerpc.c @@ -200,7 +200,9 @@ static struct blocklevel_device *arch_init_blocklevel(const char *file, bool kee return NULL; } - file_init_path(file ? file : real_file, NULL, keep_alive, &new_bl); + rc = file_init_path(file ? file : real_file, NULL, keep_alive, &new_bl); + if (rc) + new_bl = NULL; free(real_file); return new_bl; } diff --git a/external/common/arch_flash_x86.c b/external/common/arch_flash_x86.c index 3be05df..0146243 100644 --- a/external/common/arch_flash_x86.c +++ b/external/common/arch_flash_x86.c @@ -30,6 +30,7 @@ int arch_flash_init(struct blocklevel_device **r_bl, const char *file, bool keep_alive) { + int rc; struct blocklevel_device *new_bl; /* Must have passed through a file to operate on */ @@ -38,8 +39,8 @@ int arch_flash_init(struct blocklevel_device **r_bl, const char *file, bool keep return -1; } - file_init_path(file, NULL, keep_alive, &new_bl); - if (!new_bl) + rc = file_init_path(file, NULL, keep_alive, &new_bl); + if (rc) return -1; *r_bl = new_bl;
The arch_flash_init() in arch_flash_x86.c doesn't actually check the return value of file_init_path(), rather it is comparing the returned structure against NULL. It is unsafe (and incorrect at the moment) to assume that file_init_path will NULL this value on failure, it doesn't have to as it returns a value to indicate success or failure. The arch_flash_init() in arch_flash_powerpc.c calls file_init_path() through another function which will return a pointer (or NULL on failure), this function doesn't explicitly NULL its return pointer in the case that file_init_path() fails. It has initialised the pointer to NULL so the case may be less severe (compared to the arch_flash_x86 problem) as file_init_path() shouldn't have changed it on failure case, however, assuming that it won't is unsafe. It is best to explicitly NULL the return pointer if file_init_path() returns a failure. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- external/common/arch_flash_powerpc.c | 4 +++- external/common/arch_flash_x86.c | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-)