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

1380 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 first checking if the char array return from
utils::secure_getenv() is empty before creating a std::string from it.

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

Hi Niklas,

Thank you for the patch.

On Mon, Jun 10, 2019 at 03:45:31PM +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 first checking if the char array return from
> utils::secure_getenv() is empty before creating a std::string from it.

Oops :-S

std::string::string():

"Constructs the string with the contents initialized with a copy of the
null-terminated character string pointed to by s. The length of the
string is determined by the first null character. The behavior is
undefined if [s, s + Traits::length(s)) is not a valid range (for
example, if s is a null pointer)."

Shame they didn't specify that passing a nullptr would construct an
empty string.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/ipa_manager.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index f689aa69b7284092..7ea7a8cbff5f15a0 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -34,10 +34,11 @@ IPAManager::IPAManager()
>  {
>  	addDir(IPA_MODULE_DIR);
>  
> -	std::string modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> -	if (modulePaths.empty())
> +	const char *envModulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> +	if (!envModulePaths)
>  		return;
>  
> +	std::string modulePaths = envModulePaths;
>  	for (size_t pos = 0, delim = 0; delim != std::string::npos;
>  	     pos = delim + 1) {
>  		delim = modulePaths.find(':', pos);

If we want to avoid the conversion to an std::string, we could also try
the (untested) following code. Up to you.

diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index f689aa69b728..b9b772d1d5e7 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());
+
+		if (*delim == '\0')
+			break;
+
+		modulePaths += count + 1;
 	}
 }