| Message ID | 20190718050617.29455-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Jul 18, 2019 at 06:06:17AM +0100, Kieran Bingham wrote: > The IPAModule::loadIPAModuleInfo() function includes a *data pointer > which is used as a null-pointer comparison in the error path with a > conditional statement of "if (ret || !data)". > > The data variable is not initialised, and a single error path evaluates > this as "if (true || uninitialised)". > > Whilst this error path does not incorrectly utilise the uninitialised > data, as the ret evaluates to true already, it does leave a statement > which includes an uninitialised variable. > > Help the static anlaysers by initialising the data variable when it is > defined. Have you found this with any static initialiser ? Does valgrind report this issue ? > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/ipa_module.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp > index 003611625214..2ddb02c1562e 100644 > --- a/src/libcamera/ipa_module.cpp > +++ b/src/libcamera/ipa_module.cpp > @@ -291,7 +291,7 @@ int IPAModule::loadIPAModuleInfo() > return ret; > } > > - void *data; > + void *data = NULL; This should be nullptr. > size_t dataSize; > void *map; > size_t soSize;
On 18/07/2019 15:04, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Jul 18, 2019 at 06:06:17AM +0100, Kieran Bingham wrote: >> The IPAModule::loadIPAModuleInfo() function includes a *data pointer >> which is used as a null-pointer comparison in the error path with a >> conditional statement of "if (ret || !data)". >> >> The data variable is not initialised, and a single error path evaluates >> this as "if (true || uninitialised)". >> >> Whilst this error path does not incorrectly utilise the uninitialised >> data, as the ret evaluates to true already, it does leave a statement >> which includes an uninitialised variable. >> >> Help the static anlaysers by initialising the data variable when it is s/anlaysers/analysers/ >> defined. > > Have you found this with any static initialiser ? Does valgrind report > this issue ? These issues were found with clang-analyser. That was going to be detailed in the cover letter that I didn't write :-D There's one more fault in the options parsing code shared between cam and qcam, but I need to look deeper into the cause / reasoning of that one. I don't think valgrind would report this issue, as I don't think it will be an issue at runtime. The expression simply evaluates as true regardless of the uninitialised data, because the only case it can occur is when ret is already non-zero. >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/ipa_module.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp >> index 003611625214..2ddb02c1562e 100644 >> --- a/src/libcamera/ipa_module.cpp >> +++ b/src/libcamera/ipa_module.cpp >> @@ -291,7 +291,7 @@ int IPAModule::loadIPAModuleInfo() >> return ret; >> } >> >> - void *data; >> + void *data = NULL; > > This should be nullptr. Indeed it should! > >> size_t dataSize; >> void *map; >> size_t soSize; >
diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp index 003611625214..2ddb02c1562e 100644 --- a/src/libcamera/ipa_module.cpp +++ b/src/libcamera/ipa_module.cpp @@ -291,7 +291,7 @@ int IPAModule::loadIPAModuleInfo() return ret; } - void *data; + void *data = NULL; size_t dataSize; void *map; size_t soSize;
The IPAModule::loadIPAModuleInfo() function includes a *data pointer which is used as a null-pointer comparison in the error path with a conditional statement of "if (ret || !data)". The data variable is not initialised, and a single error path evaluates this as "if (true || uninitialised)". Whilst this error path does not incorrectly utilise the uninitialised data, as the ret evaluates to true already, it does leave a statement which includes an uninitialised variable. Help the static anlaysers by initialising the data variable when it is defined. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/ipa_module.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)