[libgpiod][PATCH 0/4] Fix issues detected by static analyzer

[libgpiod][PATCH 0/4] Fix issues detected by static analyzer

[Iker Pedrosa]

This patch series contain a set of fixes for several issues detected by a
static analyzer tool. They are related to wrong pointers management and
string termination. 

Iker Pedrosa (4):
  bindings: python: gpiod: avoid use after free
  lib: line-info strings termination
  lib: chip-info strings termination
  tools: free to avoid leak

 bindings/python/gpiod/ext/chip.c | 6 ++++--
 lib/chip-info.c                  | 9 ++++++---
 lib/line-info.c                  | 6 ++++--
 tools/gpioinfo.c                 | 4 +++-
 4 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.45.2

[libgpiod][PATCH 1/4] bindings: python: gpiod: avoid use after free

[Iker Pedrosa]

`req_cfg` variable is freed and then used, which would generate an
error. Avoid this problem by freeing when the variable will no longer be
used.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 bindings/python/gpiod/ext/chip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/bindings/python/gpiod/ext/chip.c b/bindings/python/gpiod/ext/chip.c
index 28cf504..e8eaad8 100644
--- a/bindings/python/gpiod/ext/chip.c
+++ b/bindings/python/gpiod/ext/chip.c
@@ -274,14 +274,16 @@ static PyObject *chip_request_lines(chip_object *self, PyObject *args)
 	Py_BEGIN_ALLOW_THREADS;
 	request = gpiod_chip_request_lines(self->chip, req_cfg, line_cfg);
 	Py_END_ALLOW_THREADS;
-	gpiod_request_config_free(req_cfg);
-	if (!request)
+	if (!request) {
+		gpiod_request_config_free(req_cfg);
 		return Py_gpiod_SetErrFromErrno();
+	}
 
 	req_obj = Py_gpiod_MakeRequestObject(request,
 			gpiod_request_config_get_event_buffer_size(req_cfg));
 	if (!req_obj)
 		gpiod_line_request_release(request);
+	gpiod_request_config_free(req_cfg);
 
 	return req_obj;
 }
-- 
2.45.2

[libgpiod][PATCH 2/4] lib: line-info strings termination

[Iker Pedrosa]

strncpy() truncates the destination buffer if it isn't large enough to
hold the copy. Thus, let's terminate the strings with a NULL character
at the end.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 lib/line-info.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/line-info.c b/lib/line-info.c
index 9f53b04..2ded9ea 100644
--- a/lib/line-info.c
+++ b/lib/line-info.c
@@ -148,10 +148,12 @@ gpiod_line_info_from_uapi(struct gpio_v2_line_info *uapi_info)
 	memset(info, 0, sizeof(*info));
 
 	info->offset = uapi_info->offset;
-	strncpy(info->name, uapi_info->name, GPIO_MAX_NAME_SIZE);
+	strncpy(info->name, uapi_info->name, GPIO_MAX_NAME_SIZE - 1);
+	info->name[GPIO_MAX_NAME_SIZE - 1] = '\0';
 
 	info->used = !!(uapi_info->flags & GPIO_V2_LINE_FLAG_USED);
-	strncpy(info->consumer, uapi_info->consumer, GPIO_MAX_NAME_SIZE);
+	strncpy(info->consumer, uapi_info->consumer, GPIO_MAX_NAME_SIZE - 1);
+	info->consumer[GPIO_MAX_NAME_SIZE - 1] = '\0';
 
 	if (uapi_info->flags & GPIO_V2_LINE_FLAG_OUTPUT)
 		info->direction = GPIOD_LINE_DIRECTION_OUTPUT;
-- 
2.45.2

Re: [libgpiod][PATCH 2/4] lib: line-info strings termination

[Kent Gibson]

On Wed, Jul 17, 2024 at 01:36:42PM +0200, Iker Pedrosa wrote:
> strncpy() truncates the destination buffer if it isn't large enough to
> hold the copy. Thus, let's terminate the strings with a NULL character
> at the end.
>
> Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
> ---
>  lib/line-info.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/line-info.c b/lib/line-info.c
> index 9f53b04..2ded9ea 100644
> --- a/lib/line-info.c
> +++ b/lib/line-info.c
> @@ -148,10 +148,12 @@ gpiod_line_info_from_uapi(struct gpio_v2_line_info *uapi_info)
>  	memset(info, 0, sizeof(*info));
>
>  	info->offset = uapi_info->offset;
> -	strncpy(info->name, uapi_info->name, GPIO_MAX_NAME_SIZE);
> +	strncpy(info->name, uapi_info->name, GPIO_MAX_NAME_SIZE - 1);
> +	info->name[GPIO_MAX_NAME_SIZE - 1] = '\0';
>

Given that uapi_info->name is not NULL terminated, this change can
incorrectly discard one character.  The correct solution is to increase
the size of info->name to allow for the NULL terminator, which would
automatically be initialised by the memset.

>  	info->used = !!(uapi_info->flags & GPIO_V2_LINE_FLAG_USED);
> -	strncpy(info->consumer, uapi_info->consumer, GPIO_MAX_NAME_SIZE);
> +	strncpy(info->consumer, uapi_info->consumer, GPIO_MAX_NAME_SIZE - 1);
> +	info->consumer[GPIO_MAX_NAME_SIZE - 1] = '\0';
>

Same here.

And same in patch 3.

Patches 1 and 4 look ok to me.

Cheers,
Kent.

>  	if (uapi_info->flags & GPIO_V2_LINE_FLAG_OUTPUT)
>  		info->direction = GPIOD_LINE_DIRECTION_OUTPUT;
> --
> 2.45.2
>

[libgpiod][PATCH 3/4] lib: chip-info strings termination

[Iker Pedrosa]

strncpy() truncates the destination buffer if it isn't large enough to
hold the copy. Thus, let's terminate the strings with a NULL character
at the end.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 lib/chip-info.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/chip-info.c b/lib/chip-info.c
index 87fd9e7..2f2523e 100644
--- a/lib/chip-info.c
+++ b/lib/chip-info.c
@@ -57,7 +57,8 @@ gpiod_chip_info_from_uapi(struct gpiochip_info *uapi_info)
 	 * GPIO device must have a name - don't bother checking this field. In
 	 * the worst case (would have to be a weird kernel bug) it'll be empty.
 	 */
-	strncpy(info->name, uapi_info->name, sizeof(info->name));
+	strncpy(info->name, uapi_info->name, sizeof(info->name) - 1);
+	info->name[sizeof(info->name) - 1] = '\0';
 
 	/*
 	 * The kernel sets the label of a GPIO device to "unknown" if it
@@ -66,8 +67,10 @@ gpiod_chip_info_from_uapi(struct gpiochip_info *uapi_info)
 	 */
 	if (uapi_info->label[0] == '\0')
 		strncpy(info->label, "unknown", sizeof(info->label));
-	else
-		strncpy(info->label, uapi_info->label, sizeof(info->label));
+	else {
+		strncpy(info->label, uapi_info->label, sizeof(info->label) - 1);
+		info->label[sizeof(info->label) - 1] = '\0';
+	}
 
 	return info;
 }
-- 
2.45.2

[libgpiod][PATCH 4/4] tools: free to avoid leak

[Iker Pedrosa]

`info` variable is allocated, but never freed when the loop continues.
Free it so that it isn't leaked.

Signed-off-by: Iker Pedrosa <ikerpedrosam@gmail.com>
---
 tools/gpioinfo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/gpioinfo.c b/tools/gpioinfo.c
index 44d1c8c..d5e4751 100644
--- a/tools/gpioinfo.c
+++ b/tools/gpioinfo.c
@@ -195,8 +195,10 @@ static void list_lines(struct line_resolver *resolver, struct gpiod_chip *chip,
 				   offset, gpiod_chip_info_get_name(chip_info));
 
 		if (resolver->num_lines &&
-		    !resolve_line(resolver, info, chip_num))
+		    !resolve_line(resolver, info, chip_num)) {
+			gpiod_line_info_free(info);
 			continue;
+		}
 
 		if (resolver->num_lines) {
 			printf("%s %u", gpiod_chip_info_get_name(chip_info),
-- 
2.45.2

Re: [libgpiod][PATCH 0/4] Fix issues detected by static analyzer

[Bartosz Golaszewski]

On Wed, Jul 17, 2024 at 1:37 PM Iker Pedrosa <ikerpedrosam@gmail.com> wrote:
>
> This patch series contain a set of fixes for several issues detected by a
> static analyzer tool. They are related to wrong pointers management and
> string termination.
>

What is the static analyzer you used for this?

Bart

> Iker Pedrosa (4):
>   bindings: python: gpiod: avoid use after free
>   lib: line-info strings termination
>   lib: chip-info strings termination
>   tools: free to avoid leak
>
>  bindings/python/gpiod/ext/chip.c | 6 ++++--
>  lib/chip-info.c                  | 9 ++++++---
>  lib/line-info.c                  | 6 ++++--
>  tools/gpioinfo.c                 | 4 +++-
>  4 files changed, 17 insertions(+), 8 deletions(-)
>
> --
> 2.45.2
>