Fix issues detected by static analyzer

PATCH Superseded Iker Pedrosa 3 participants 7 messages Jul 17, 2024 – Jul 19, 2024

Subsystems (2)

Library code GPIO

lore

Share & tools

lore

Iker Pedrosa

author

2024-07-17 11:37 UTC #0 lore raw

cover Fix issues detected by static analyzer

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(-)

Signature · 1 line

2.45.2

Iker Pedrosa

author

2024-07-17 11:37 UTC #1 lore raw

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

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 <[email protected]>

---
 bindings/python/gpiod/ext/chip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

bindings/python/gpiod/ext/chip.c +4 -2

index 28cf504..e8eaad8 100644
@@ -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;
 }

Signature · 1 line

2.45.2

Iker Pedrosa

author

2024-07-17 11:38 UTC #2 lore raw

PATCH 2/4 lib: line-info strings termination

In reply to 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 <[email protected]>

---
 lib/line-info.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

lib/line-info.c +4 -2

index 9f53b04..2ded9ea 100644
@@ -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;

Signature · 1 line

2.45.2

Kent Gibson

2024-07-17 15:11 UTC #3 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 lib/line-info.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
lib/line-info.c +0 -0
index 9f53b04..2ded9ea 100644

lib/line-info.c gpiod_line_info_from_uapi(struct gpio_v2_line_info *uapi_info) · line 153

Quoted 7 lines

lib/line-info.c +2 -1

@@ -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.

lib/line-info.c gpiod_line_info_from_uapi(struct gpio_v2_line_info *uapi_info) · line 157

Quoted 5 lines

lib/line-info.c +2 -1

 	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.

Quoted 5 lines

lib/line-info.c +0 -1

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

2.45.2

Iker Pedrosa

author

2024-07-17 11:38 UTC #4 lore raw

PATCH 3/4 lib: chip-info strings termination

In reply to 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 <[email protected]>

---
 lib/chip-info.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

lib/chip-info.c +6 -3

index 87fd9e7..2f2523e 100644
@@ -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;
 }

Signature · 1 line

2.45.2

Iker Pedrosa

author

2024-07-17 11:38 UTC #5 lore raw

PATCH 4/4 tools: free to avoid leak

In reply to 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 <[email protected]>

---
 tools/gpioinfo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

tools/gpioinfo.c +3 -1

index 44d1c8c..d5e4751 100644
@@ -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),

Signature · 1 line

2.45.2

Bartosz Golaszewski

2024-07-19 09:33 UTC #6 lore raw

In reply to Iker Pedrosa

Quoted 5 lines · 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.

What is the static analyzer you used for this?

Bart

Quoted 15 lines

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