[libcamera-devel,v2] libcamera: ipa_manager: Fix handling of unset LIBCAMERA_IPA_MODULE_PATH

1388 diff mbox series

If the environment variable LIBCAMERA_IPA_MODULE_PATH is not set
utils::secure_getenv() will return a nullptr. Assigning a nullptr to a
std::string is not valid and results in a crash,

    terminate called after throwing an instance of 'std::logic_error'
      what():  basic_string::_M_construct null not valid

Fix this by operating directly on the returned char array instead of
turning it into a std::string.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/ipa_manager.cpp | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Hi Niklas,

Thank you for the patch.

On Mon, Jun 10, 2019 at 11:00:23PM +0200, Niklas Söderlund wrote:
> If the environment variable LIBCAMERA_IPA_MODULE_PATH is not set
> utils::secure_getenv() will return a nullptr. Assigning a nullptr to a
> std::string is not valid and results in a crash,
> 
>     terminate called after throwing an instance of 'std::logic_error'
>       what():  basic_string::_M_construct null not valid
> 
> Fix this by operating directly on the returned char array instead of
> turning it into a std::string.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/ipa_manager.cpp | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f689aa69b7284092..b9b772d1d5e7795f 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -34,19 +34,22 @@ IPAManager::IPAManager()
>  {
>  	addDir(IPA_MODULE_DIR);
>  
> -	std::string modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> -	if (modulePaths.empty())
> +	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> +	if (!modulePaths)
>  		return;
>  
> -	for (size_t pos = 0, delim = 0; delim != std::string::npos;
> -	     pos = delim + 1) {
> -		delim = modulePaths.find(':', pos);
> -		size_t count = delim == std::string::npos ? delim : delim - pos;
> -		std::string path = modulePaths.substr(pos, count);
> -		if (path.empty())
> -			continue;
> +	while (1) {
> +		const char *delim = strchrnul(modulePaths, ':');
> +		size_t count = delim - modulePaths;
>  
> -		addDir(path.c_str());
> +		std::string path(modulePaths, count);
> +		if (!path.empty())
> +			addDir(path.c_str());

Small improvement, to avoid constructing an empty string,

		if (count) {
			std::string path(modulePaths, count);
			addDir(path.c_str());
		}

Could you test this with a LIBCAMERA_IPA_MODULE_PATH that contains "::"
and that ends with ":" ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +		if (*delim == '\0')
> +			break;
> +
> +		modulePaths += count + 1;
>  	}
>  }
>

Hi Laurent,

Thanks for your feedback.

On 2019-06-11 13:17:10 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Mon, Jun 10, 2019 at 11:00:23PM +0200, Niklas Söderlund wrote:
> > If the environment variable LIBCAMERA_IPA_MODULE_PATH is not set
> > utils::secure_getenv() will return a nullptr. Assigning a nullptr to a
> > std::string is not valid and results in a crash,
> > 
> >     terminate called after throwing an instance of 'std::logic_error'
> >       what():  basic_string::_M_construct null not valid
> > 
> > Fix this by operating directly on the returned char array instead of
> > turning it into a std::string.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/ipa_manager.cpp | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > index f689aa69b7284092..b9b772d1d5e7795f 100644
> > --- a/src/libcamera/ipa_manager.cpp
> > +++ b/src/libcamera/ipa_manager.cpp
> > @@ -34,19 +34,22 @@ IPAManager::IPAManager()
> >  {
> >  	addDir(IPA_MODULE_DIR);
> >  
> > -	std::string modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> > -	if (modulePaths.empty())
> > +	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> > +	if (!modulePaths)
> >  		return;
> >  
> > -	for (size_t pos = 0, delim = 0; delim != std::string::npos;
> > -	     pos = delim + 1) {
> > -		delim = modulePaths.find(':', pos);
> > -		size_t count = delim == std::string::npos ? delim : delim - pos;
> > -		std::string path = modulePaths.substr(pos, count);
> > -		if (path.empty())
> > -			continue;
> > +	while (1) {
> > +		const char *delim = strchrnul(modulePaths, ':');
> > +		size_t count = delim - modulePaths;
> >  
> > -		addDir(path.c_str());
> > +		std::string path(modulePaths, count);
> > +		if (!path.empty())
> > +			addDir(path.c_str());
> 
> Small improvement, to avoid constructing an empty string,
> 
> 		if (count) {
> 			std::string path(modulePaths, count);
> 			addDir(path.c_str());
> 		}
> 
> Could you test this with a LIBCAMERA_IPA_MODULE_PATH that contains "::"
> and that ends with ":" ?

It works.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, pushed to master with your tag.

> 
> > +
> > +		if (*delim == '\0')
> > +			break;
> > +
> > +		modulePaths += count + 1;
> >  	}
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart