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

[Niklas Söderlund]

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 at 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 at ideasonboard.com>

Thanks, pushed to master with your tag.

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

-- 
Regards,
Niklas Söderlund