From eed04bd3418b860de60dd79e0a7be13cf8ec7829 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 13:27:46 -0400 Subject: [PATCH 01/10] channel.c: track length of buffer when buidling xml As we are building up the xml, keep track of the length of the (remaining) buffer, and check it at the end to make sure we didn't overflow. This does change the max length of the channel xml description from MAX_size_t to MAX_ssize_t. Worse case, that is from 64k to 32k. The C spec defines the minimum size_t to 16-bits. Nominally, on most modern compilers (where size_t is 32-bits) this would reduce things from 4G to 2G. On Pluto, the largest is 1280 bytes, M2k is 492, so, even 32k seems pretty large. Signed-off-by: Robin Getz --- channel.c | 25 +++++++++++++++++++++---- iio-private.h | 3 +++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/channel.c b/channel.c index 407ce632f..a38448b9b 100644 --- a/channel.c +++ b/channel.c @@ -226,13 +226,16 @@ static char * get_scan_element(const struct iio_channel *chn, size_t *length) /* Returns a string containing the XML representation of this channel */ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) { - size_t len = sizeof("") - + strlen(chn->id) + (chn->name ? strlen(chn->name) : 0); - char *ptr, *str, **attrs, *scan_element = NULL; + ssize_t len; + char *ptr, *eptr, *str, **attrs, *scan_element = NULL; size_t *attrs_len, scan_element_len = 0; unsigned int i; + len = sizeof(""); + len += strnlen(chn->id, MAX_CHN_ID); + len += chn->is_output ? sizeof("output") : sizeof("input"); + len += chn->name ? sizeof(" name= ") + strnlen(chn->name, MAX_CHN_NAME) : 0; + if (chn->is_scan_element) { scan_element = get_scan_element(chn, &scan_element_len); if (!scan_element) @@ -260,26 +263,32 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) str = malloc(len); if (!str) goto err_free_attrs; + eptr = str + len; iio_snprintf(str, len, "id); ptr = strrchr(str, '\0'); + len = eptr - ptr; if (chn->name) { sprintf(ptr, " name=\"%s\"", chn->name); ptr = strrchr(ptr, '\0'); + len = eptr - ptr; } sprintf(ptr, " type=\"%s\" >", chn->is_output ? "output" : "input"); ptr = strrchr(ptr, '\0'); + len = eptr - ptr; if (chn->is_scan_element) { strcpy(ptr, scan_element); ptr += scan_element_len; + len -= scan_element_len; } for (i = 0; i < chn->nb_attrs; i++) { strcpy(ptr, attrs[i]); ptr += attrs_len[i]; + len -= attrs_len[i]; free(attrs[i]); } @@ -289,6 +298,14 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) strcpy(ptr, ""); *length = ptr - str + sizeof("") - 1; + len -= sizeof(""); + + if (len < 0) { + IIO_ERROR("Internal libIIO error: iio_channel_get_xml str length isssue\n"); + free(str); + return NULL; + } + return str; err_free_attrs: diff --git a/iio-private.h b/iio-private.h index 7077bbae6..79232979f 100644 --- a/iio-private.h +++ b/iio-private.h @@ -60,6 +60,9 @@ #define CLEAR_BIT(addr, bit) \ *(((uint32_t *) addr) + BIT_WORD(bit)) &= ~BIT_MASK(bit) +/* 256 is the MAX_NAME (file name) on Linux, 4096 is PAGESIZE */ +#define MAX_CHN_ID 256 /* encoded in the sysfs filename */ +#define MAX_CHN_NAME 256 /* encoded in the sysfs filename */ /* ntohl/htonl are a nightmare to use in cross-platform applications, * since they are defined in different headers on different platforms. From 8c83eaed710ff89a2159d97a27f03a21a427492f Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 15:49:34 -0400 Subject: [PATCH 02/10] device.c track length of buffer when buidling xml As we are building up the xml, keep track of the length of the (remaining) buffer, and check it at the end to make sure we didn't overflow. This does change the max length of the channel xml description from MAX_size_t to MAX_ssize_t. Worse case, that is from 64k to 32k. The C spec defines the minimum size_t to 16-bits. Nominally, on most modern compilers (where size_t is 32-bits) this would reduce things from 4G to 2G. On Pluto, the largest is 8884 bytes, M2k is 17062, so, even 32k seems pretty large. Signed-off-by: Robin Getz --- device.c | 29 ++++++++++++++++++++++++++--- iio-private.h | 2 ++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/device.c b/device.c index d3ec781c6..9787434b5 100644 --- a/device.c +++ b/device.c @@ -65,12 +65,18 @@ static char *get_attr_xml(const char *attr, size_t *length, enum iio_attr_type t /* Returns a string containing the XML representation of this device */ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) { - size_t len = sizeof("") - + strlen(dev->id) + (dev->name ? strlen(dev->name) : 0); - char *ptr, *str, **attrs, **channels, **buffer_attrs, **debug_attrs; + ssize_t len; + char *ptr, *eptr, *str, **attrs, **channels, **buffer_attrs, **debug_attrs; size_t *attrs_len, *channels_len, *buffer_attrs_len, *debug_attrs_len; unsigned int i, j, k; + len = sizeof(" ") - 1; + len += strnlen(dev->id, MAX_DEV_ID); + if (dev->name) { + len += sizeof(" name=\"\"") - 1; + len += strnlen(dev->name, MAX_DEV_NAME); + } + attrs_len = malloc(dev->nb_attrs * sizeof(*attrs_len)); if (!attrs_len) return NULL; @@ -143,21 +149,26 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) str = malloc(len); if (!str) goto err_free_debug_attrs; + eptr = str + len; iio_snprintf(str, len, "id); ptr = strrchr(str, '\0'); + len = eptr - ptr; if (dev->name) { sprintf(ptr, " name=\"%s\"", dev->name); ptr = strrchr(ptr, '\0'); + len = eptr - ptr; } strcpy(ptr, " >"); ptr += 2; + len -= 2; for (i = 0; i < dev->nb_channels; i++) { strcpy(ptr, channels[i]); ptr += channels_len[i]; + len -= channels_len[i]; free(channels[i]); } @@ -167,6 +178,7 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) for (i = 0; i < dev->nb_attrs; i++) { strcpy(ptr, attrs[i]); ptr += attrs_len[i]; + len -= attrs_len[i]; free(attrs[i]); } @@ -176,6 +188,7 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) for (i = 0; i < dev->nb_buffer_attrs; i++) { strcpy(ptr, buffer_attrs[i]); ptr += buffer_attrs_len[i]; + len -= buffer_attrs_len[i]; free(buffer_attrs[i]); } @@ -185,6 +198,7 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) for (i = 0; i < dev->nb_debug_attrs; i++) { strcpy(ptr, debug_attrs[i]); ptr += debug_attrs_len[i]; + len -= debug_attrs_len[i]; free(debug_attrs[i]); } @@ -192,7 +206,16 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(debug_attrs_len); strcpy(ptr, ""); + len -= sizeof("") - 1; + *length = ptr - str + sizeof("") - 1; + + if (len < 0) { + IIO_ERROR("Internal libIIO error: iio_device_get_xml str length isssue\n"); + free(str); + return NULL; + } + return str; err_free_debug_attrs: diff --git a/iio-private.h b/iio-private.h index 79232979f..1a5f2e543 100644 --- a/iio-private.h +++ b/iio-private.h @@ -63,6 +63,8 @@ /* 256 is the MAX_NAME (file name) on Linux, 4096 is PAGESIZE */ #define MAX_CHN_ID 256 /* encoded in the sysfs filename */ #define MAX_CHN_NAME 256 /* encoded in the sysfs filename */ +#define MAX_DEV_ID 256 /* encoded in the sysfs filename */ +#define MAX_DEV_NAME 256 /* encoded in the sysfs filename */ /* ntohl/htonl are a nightmare to use in cross-platform applications, * since they are defined in different headers on different platforms. From 36204e284bff9d04379d0ba9b913e602cae3245c Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 16:06:26 -0400 Subject: [PATCH 03/10] context.c : track length of buffer when buidling xml As we are building up the xml, keep track of the length of the (remaining) buffer, and check it at the end to make sure we didn't overflow. This does change the max length of the channel xml description from MAX_size_t to MAX_ssize_t. Worse case, that is from 64k to 32k. The C spec defines the minimum size_t to 16-bits. Nominally, on most modern compilers (where size_t is 32-bits) this would reduce things from 4G to 2G. On Pluto, the nomial context is 24962 bytes, M2k is 31178. 32k seems pretty thin. Good thing we are all on modern compilers. Signed-off-by: Robin Getz --- context.c | 43 ++++++++++++++++++++++++++++++------------- iio-private.h | 4 ++++ 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/context.c b/context.c index c12395c3d..683e04d2f 100644 --- a/context.c +++ b/context.c @@ -53,20 +53,25 @@ static const char xml_header[] = "" /* Returns a string containing the XML representation of this context */ char * iio_context_create_xml(const struct iio_context *ctx) { - size_t len, *devices_len = NULL; - char *str, *ptr, **devices = NULL; + ssize_t len; + size_t *devices_len = NULL; + char *str, *ptr, *eptr, **devices = NULL; unsigned int i; - len = strlen(ctx->name) + sizeof(xml_header) - 1 + - sizeof(""); - if (ctx->description) - len += strlen(ctx->description) + - sizeof(" description=\"\"") - 1; + len = sizeof(xml_header) - 1; + len += strnlen(ctx->name, MAX_CTX_NAME); + len += sizeof("") - 1; - for (i = 0; i < ctx->nb_attrs; i++) - len += strlen(ctx->attrs[i]) + - strlen(ctx->values[i]) + - sizeof(""); + if (ctx->description) { + len += strnlen(ctx->description, MAX_CTX_DESC); + len += sizeof(" description=\"\"") - 1; + } + + for (i = 0; i < ctx->nb_attrs; i++) { + len += strnlen(ctx->attrs[i], MAX_ATTR_NAME); + len += strnlen(ctx->values[i], MAX_ATTR_VALUE); + len += sizeof("") - 1; + } if (ctx->nb_devices) { devices_len = malloc(ctx->nb_devices * sizeof(*devices_len)); @@ -94,6 +99,7 @@ char * iio_context_create_xml(const struct iio_context *ctx) errno = ENOMEM; goto err_free_devices; } + eptr = str + len; if (ctx->description) { iio_snprintf(str, len, "%snb_attrs; i++) + for (i = 0; i < ctx->nb_attrs; i++) { ptr += sprintf(ptr, "", ctx->attrs[i], ctx->values[i]); - + len = eptr - ptr; + } for (i = 0; i < ctx->nb_devices; i++) { strcpy(ptr, devices[i]); ptr += devices_len[i]; + len -= devices_len[i]; free(devices[i]); } free(devices); free(devices_len); strcpy(ptr, ""); + len -= sizeof("") - 1; + + if (len < 0) { + IIO_ERROR("Internal libIIO error: iio_context_create_xml str length isssue\n"); + free(str); + return NULL; + } + return str; err_free_devices: diff --git a/iio-private.h b/iio-private.h index 1a5f2e543..bd02f71cc 100644 --- a/iio-private.h +++ b/iio-private.h @@ -65,6 +65,10 @@ #define MAX_CHN_NAME 256 /* encoded in the sysfs filename */ #define MAX_DEV_ID 256 /* encoded in the sysfs filename */ #define MAX_DEV_NAME 256 /* encoded in the sysfs filename */ +#define MAX_CTX_NAME 256 /* nominally "xml" */ +#define MAX_CTX_DESC 256 /* nominally "linux ..." */ +#define MAX_ATTR_NAME 256 /* encoded in the sysfs filename */ +#define MAX_ATTR_VALUE 4096 /* Linux page size, could be anything */ /* ntohl/htonl are a nightmare to use in cross-platform applications, * since they are defined in different headers on different platforms. From 474c708d3104c7e141a73f28d568934b7ac5d77e Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 16:18:22 -0400 Subject: [PATCH 04/10] utilities.c: add an iio_strlcpy, a "safer" version of strncpy Add a private verison of strlcpy, from FreeBSD (Todd C. Miller). strlcpy() is guarantee to NUL-terminate the resulting dest, even if it is too small. Also note that strlcpy() and strlcat() only operate on true 'C' strings. This means that for strlcpy(), src must be NUL-terminated. This is considered by many to be safer, and less error prone that strncpy, which does not NUL-terminate on truncation. Signed-off-by: Robin Getz --- iio-private.h | 1 + utilities.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/iio-private.h b/iio-private.h index bd02f71cc..dafcfd841 100644 --- a/iio-private.h +++ b/iio-private.h @@ -298,6 +298,7 @@ void iio_channel_init_finalize(struct iio_channel *chn); unsigned int find_channel_modifier(const char *s, size_t *len_p); char *iio_strdup(const char *str); +size_t iio_strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize); int iio_context_add_attr(struct iio_context *ctx, const char *key, const char *value); diff --git a/utilities.c b/utilities.c index 85a4813cc..2178f194c 100644 --- a/utilities.c +++ b/utilities.c @@ -214,3 +214,46 @@ char *iio_strdup(const char *str) return buf; #endif } + +/* strlcpy is designed to be safer, more consistent, and less error prone + * replacements for strncpy, since it guarantees to NUL-terminate the result. + * + * This function + * Copyright (c) 1998, 2015 Todd C. Miller + * https://github.com/freebsd/freebsd/blob/master/sys/libkern/strlcpy.c + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * Copy string src to buffer dst of size dsize. At most dsize-1 + * chars will be copied. Always NUL terminates (unless dsize == 0). + * Returns strlen(src); if retval >= dsize, truncation occurred. + * + * src is assumed to be null terminated, if it is not, this function will + * dereference unknown memory beyond src. + */ +size_t iio_strlcpy(char * __restrict dst, const char * __restrict src, size_t dsize) +{ + const char *osrc = src; + size_t nleft = dsize; + + /* Copy as many bytes as will fit. */ + if (nleft != 0) { + while (--nleft != 0) { + if ((*dst++ = *src++) == '\0') + break; + } + } + + /* Not enough room in dst, add NUL and traverse rest of src. */ + if (nleft == 0) { + if (dsize != 0) + *dst = '\0'; /* NUL-terminate dst */ + + while (*src++) + ; + } + + return(src - osrc - 1); /* count does not include NUL */ +} From 1e91e5da341794175fea4b80ff970615d1e6a7f0 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 16:47:01 -0400 Subject: [PATCH 05/10] channel.c: remove strcpy & sprintf, and move to safe functions We used strcpy & sprintf in a few places, which opens up classic buffer overflow issues, as described in: https://cwe.mitre.org/data/definitions/120.html This adds the recent length checking, to make sure as the buffer descreases in size, it is managed properly. If we ever think we run out of space, we will no longer buffer overflow. Tested on Pluto and M2k to see if this introduces issues, and couldn't find anything. Signed-off-by: Robin Getz --- channel.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/channel.c b/channel.c index a38448b9b..e484f7532 100644 --- a/channel.c +++ b/channel.c @@ -263,32 +263,39 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) str = malloc(len); if (!str) goto err_free_attrs; + ptr = str; eptr = str + len; - iio_snprintf(str, len, "id); - ptr = strrchr(str, '\0'); - len = eptr - ptr; + if (len > 0) { + iio_snprintf(str, len, "id); + ptr = strrchr(str, '\0'); + len = eptr - ptr; + } - if (chn->name) { - sprintf(ptr, " name=\"%s\"", chn->name); + if (chn->name && len > 0) { + iio_snprintf(ptr, len, " name=\"%s\"", chn->name); ptr = strrchr(ptr, '\0'); len = eptr - ptr; } - sprintf(ptr, " type=\"%s\" >", chn->is_output ? "output" : "input"); - ptr = strrchr(ptr, '\0'); - len = eptr - ptr; + if (len > 0) { + iio_snprintf(ptr, len, " type=\"%s\" >", chn->is_output ? "output" : "input"); + ptr = strrchr(ptr, '\0'); + len = eptr - ptr; + } - if (chn->is_scan_element) { - strcpy(ptr, scan_element); + if (chn->is_scan_element && len > 0) { + iio_strlcpy(ptr, scan_element, len); ptr += scan_element_len; len -= scan_element_len; } for (i = 0; i < chn->nb_attrs; i++) { - strcpy(ptr, attrs[i]); - ptr += attrs_len[i]; - len -= attrs_len[i]; + if (len > 0) { + iio_strlcpy(ptr, attrs[i], len); + ptr += attrs_len[i]; + len -= attrs_len[i]; + } free(attrs[i]); } @@ -296,9 +303,11 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) free(attrs); free(attrs_len); - strcpy(ptr, ""); + if (len > 0) { + iio_strlcpy(ptr, "", len); + len -= sizeof(""); + } *length = ptr - str + sizeof("") - 1; - len -= sizeof(""); if (len < 0) { IIO_ERROR("Internal libIIO error: iio_channel_get_xml str length isssue\n"); From 2f55cd4a2af3e9597613734a442336216306eaf5 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 16:58:43 -0400 Subject: [PATCH 06/10] device.c : remove strcpy & sprintf, and move to safe functions We used strcpy & sprintf in a few places, which opens up classic buffer overflow issues, as described in: https://cwe.mitre.org/data/definitions/120.html This adds the recent length checking, to make sure as the buffer descreases in size, it is managed properly. If we ever think we run out of space, we will no longer buffer overflow. Tested on Pluto and M2k to see if this introduces issues, and couldn't find anything. Signed-off-by: Robin Getz --- device.c | 59 +++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/device.c b/device.c index 9787434b5..0fb4fa348 100644 --- a/device.c +++ b/device.c @@ -150,25 +150,32 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) if (!str) goto err_free_debug_attrs; eptr = str + len; + ptr = str; - iio_snprintf(str, len, "id); - ptr = strrchr(str, '\0'); - len = eptr - ptr; + if (len > 0) { + iio_snprintf(str, len, "id); + ptr = strrchr(str, '\0'); + len = eptr - ptr; + } - if (dev->name) { - sprintf(ptr, " name=\"%s\"", dev->name); + if (dev->name && len > 0) { + iio_snprintf(ptr, len, " name=\"%s\"", dev->name); ptr = strrchr(ptr, '\0'); len = eptr - ptr; } - strcpy(ptr, " >"); - ptr += 2; - len -= 2; + if (len > 0) { + iio_strlcpy(ptr, " >", len); + ptr += 2; + len -= 2; + } for (i = 0; i < dev->nb_channels; i++) { - strcpy(ptr, channels[i]); - ptr += channels_len[i]; - len -= channels_len[i]; + if (len > 0) { + iio_strlcpy(ptr, channels[i], len); + ptr += channels_len[i]; + len -= channels_len[i]; + } free(channels[i]); } @@ -176,9 +183,11 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(channels_len); for (i = 0; i < dev->nb_attrs; i++) { - strcpy(ptr, attrs[i]); - ptr += attrs_len[i]; - len -= attrs_len[i]; + if (len > 0) { + iio_strlcpy(ptr, attrs[i], len); + ptr += attrs_len[i]; + len -= attrs_len[i]; + } free(attrs[i]); } @@ -186,9 +195,11 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(attrs_len); for (i = 0; i < dev->nb_buffer_attrs; i++) { - strcpy(ptr, buffer_attrs[i]); - ptr += buffer_attrs_len[i]; - len -= buffer_attrs_len[i]; + if (len > 0) { + iio_strlcpy(ptr, buffer_attrs[i], len); + ptr += buffer_attrs_len[i]; + len -= buffer_attrs_len[i]; + } free(buffer_attrs[i]); } @@ -196,17 +207,21 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(buffer_attrs_len); for (i = 0; i < dev->nb_debug_attrs; i++) { - strcpy(ptr, debug_attrs[i]); - ptr += debug_attrs_len[i]; - len -= debug_attrs_len[i]; + if (len > 0) { + iio_strlcpy(ptr, debug_attrs[i], len); + ptr += debug_attrs_len[i]; + len -= debug_attrs_len[i]; + } free(debug_attrs[i]); } free(debug_attrs); free(debug_attrs_len); - strcpy(ptr, ""); - len -= sizeof("") - 1; + if (len > 0) { + iio_strlcpy(ptr, "", len); + len -= sizeof("") - 1; + } *length = ptr - str + sizeof("") - 1; From fde81b1950b5f34be707f106348943d060320d30 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 17:08:11 -0400 Subject: [PATCH 07/10] context.c: remove strcpy & sprintf, and move to safe functions We used strcpy & sprintf in a few places, which opens up classic buffer overflow issues, as described in: https://cwe.mitre.org/data/definitions/120.html This adds the recent length checking, to make sure as the buffer descreases in size, it is managed properly. If we ever think we run out of space, we will no longer buffer overflow. Tested on Pluto and M2k to see if this introduces issues, and couldn't find anything. Signed-off-by: Robin Getz --- context.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/context.c b/context.c index 683e04d2f..7ba18affd 100644 --- a/context.c +++ b/context.c @@ -100,36 +100,43 @@ char * iio_context_create_xml(const struct iio_context *ctx) goto err_free_devices; } eptr = str + len; - - if (ctx->description) { - iio_snprintf(str, len, "%s", - xml_header, ctx->name, ctx->description); - } else { - iio_snprintf(str, len, "%s", - xml_header, ctx->name); + ptr = str; + + if (len > 0) { + if (ctx->description) { + iio_snprintf(str, len, "%s", + xml_header, ctx->name, ctx->description); + } else { + iio_snprintf(str, len, "%s", + xml_header, ctx->name); + } + ptr = strrchr(str, '\0'); + len = eptr - ptr; } - ptr = strrchr(str, '\0'); - len = eptr - ptr; - - for (i = 0; i < ctx->nb_attrs; i++) { - ptr += sprintf(ptr, "", + for (i = 0; i < ctx->nb_attrs && len > 0; i++) { + ptr += iio_snprintf(ptr, len, "", ctx->attrs[i], ctx->values[i]); len = eptr - ptr; } for (i = 0; i < ctx->nb_devices; i++) { - strcpy(ptr, devices[i]); - ptr += devices_len[i]; - len -= devices_len[i]; + if (len > 0) { + iio_strlcpy(ptr, devices[i], len); + ptr += devices_len[i]; + len -= devices_len[i]; + } free(devices[i]); } free(devices); free(devices_len); - strcpy(ptr, ""); - len -= sizeof("") - 1; + + if (len > 0) { + iio_strlcpy(ptr, "", len); + len -= sizeof("") - 1; + } if (len < 0) { IIO_ERROR("Internal libIIO error: iio_context_create_xml str length isssue\n"); From 945407cbc7d54fd4505aa18c4dfcbdac954e39df Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 18:47:06 -0400 Subject: [PATCH 08/10] channel.c: optimize buidling xml function try to use memcpy when we know the source length. memcpy will operate on word or double-word, unlike iio_strlcpy which operates on a byte basis. Mark these calls as Flawfinder ignore, since we check the dest buffer length to to the source length before we do a copy. Rather than itererate over the dest string to find the end (with strrchr), just keep track with math. We know the start, and we know the length (from the outputs of iio_snprintf & iio_strlcpy), so just add them. Signed-off-by: Robin Getz --- channel.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/channel.c b/channel.c index e484f7532..10401d595 100644 --- a/channel.c +++ b/channel.c @@ -267,32 +267,29 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) eptr = str + len; if (len > 0) { - iio_snprintf(str, len, "id); - ptr = strrchr(str, '\0'); + ptr += iio_snprintf(str, len, "id); len = eptr - ptr; } if (chn->name && len > 0) { - iio_snprintf(ptr, len, " name=\"%s\"", chn->name); - ptr = strrchr(ptr, '\0'); + ptr += iio_snprintf(ptr, len, " name=\"%s\"", chn->name); len = eptr - ptr; } if (len > 0) { - iio_snprintf(ptr, len, " type=\"%s\" >", chn->is_output ? "output" : "input"); - ptr = strrchr(ptr, '\0'); + ptr += iio_snprintf(ptr, len, " type=\"%s\" >", chn->is_output ? "output" : "input"); len = eptr - ptr; } - if (chn->is_scan_element && len > 0) { - iio_strlcpy(ptr, scan_element, len); + if (chn->is_scan_element && len > scan_element_len) { + memcpy(ptr, scan_element, scan_element_len); /* Flawfinder: ignore */ ptr += scan_element_len; len -= scan_element_len; } for (i = 0; i < chn->nb_attrs; i++) { - if (len > 0) { - iio_strlcpy(ptr, attrs[i], len); + if (len > attrs_len[i]) { + memcpy(ptr, attrs[i], attrs_len[i]); /* Flawfinder: ignore */ ptr += attrs_len[i]; len -= attrs_len[i]; } @@ -304,10 +301,11 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length) free(attrs_len); if (len > 0) { - iio_strlcpy(ptr, "", len); - len -= sizeof(""); + ptr += iio_strlcpy(ptr, "", len); + len -= sizeof("") -1; } - *length = ptr - str + sizeof("") - 1; + + *length = ptr - str; if (len < 0) { IIO_ERROR("Internal libIIO error: iio_channel_get_xml str length isssue\n"); From 55842bfb1a29ef711b8453addcd90c441f0696d8 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 18:54:03 -0400 Subject: [PATCH 09/10] device.c: optimize building xml function try to use memcpy when we know the source length. memcpy will operate on word or double-word, unlike iio_strlcpy which operates on a byte basis. Mark these calls as Flawfinder ignore, since we check the dest buffer length to to the source length before we do a copy. Rather than itererate over the dest string to find the end (with strrchr), just keep track with math. We know the start, and we know the length (from the outputs of iio_snprintf & iio_strlcpy), so just add them. Signed-off-by: Robin Getz --- device.c | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/device.c b/device.c index 0fb4fa348..e847264a0 100644 --- a/device.c +++ b/device.c @@ -153,26 +153,23 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) ptr = str; if (len > 0) { - iio_snprintf(str, len, "id); - ptr = strrchr(str, '\0'); + ptr += iio_snprintf(str, len, "id); len = eptr - ptr; } if (dev->name && len > 0) { - iio_snprintf(ptr, len, " name=\"%s\"", dev->name); - ptr = strrchr(ptr, '\0'); + ptr += iio_snprintf(ptr, len, " name=\"%s\"", dev->name); len = eptr - ptr; } if (len > 0) { - iio_strlcpy(ptr, " >", len); - ptr += 2; + ptr += iio_strlcpy(ptr, " >", len); len -= 2; } for (i = 0; i < dev->nb_channels; i++) { - if (len > 0) { - iio_strlcpy(ptr, channels[i], len); + if (len > channels_len[i]) { + memcpy(ptr, channels[i], channels_len[i]); /* Flawfinder: ignore */ ptr += channels_len[i]; len -= channels_len[i]; } @@ -183,8 +180,8 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(channels_len); for (i = 0; i < dev->nb_attrs; i++) { - if (len > 0) { - iio_strlcpy(ptr, attrs[i], len); + if (len > attrs_len[i]) { + memcpy(ptr, attrs[i], attrs_len[i]); /* Flawfinder: ignore */ ptr += attrs_len[i]; len -= attrs_len[i]; } @@ -195,8 +192,8 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(attrs_len); for (i = 0; i < dev->nb_buffer_attrs; i++) { - if (len > 0) { - iio_strlcpy(ptr, buffer_attrs[i], len); + if (len > buffer_attrs_len[i]) { + memcpy(ptr, buffer_attrs[i], buffer_attrs_len[i]); /* Flawfinder: ignore */ ptr += buffer_attrs_len[i]; len -= buffer_attrs_len[i]; } @@ -207,8 +204,8 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(buffer_attrs_len); for (i = 0; i < dev->nb_debug_attrs; i++) { - if (len > 0) { - iio_strlcpy(ptr, debug_attrs[i], len); + if (len > debug_attrs_len[i]) { + memcpy(ptr, debug_attrs[i], debug_attrs_len[i]); /* Flawfinder: ignore */ ptr += debug_attrs_len[i]; len -= debug_attrs_len[i]; } @@ -219,11 +216,11 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length) free(debug_attrs_len); if (len > 0) { - iio_strlcpy(ptr, "", len); + ptr += iio_strlcpy(ptr, "", len); len -= sizeof("") - 1; } - *length = ptr - str + sizeof("") - 1; + *length = ptr - str; if (len < 0) { IIO_ERROR("Internal libIIO error: iio_device_get_xml str length isssue\n"); From 1fa30408115d104ebf9f3ab47699771e177af2b9 Mon Sep 17 00:00:00 2001 From: Robin Getz Date: Tue, 21 Apr 2020 18:56:30 -0400 Subject: [PATCH 10/10] context.c: optimize buidling xml function try to use memcpy when we know the source length. memcpy will operate on word or double-word, unlike iio_strlcpy which operates on a byte basis. Mark these calls as Flawfinder ignore, since we check the dest buffer length to to the source length before we do a copy. Rather than itererate over the dest string to find the end (with strrchr), just keep track with math. We know the start, and we know the length (from the outputs of iio_snprintf & iio_strlcpy), so just add them. Signed-off-by: Robin Getz --- context.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/context.c b/context.c index 7ba18affd..eed8730c2 100644 --- a/context.c +++ b/context.c @@ -104,14 +104,13 @@ char * iio_context_create_xml(const struct iio_context *ctx) if (len > 0) { if (ctx->description) { - iio_snprintf(str, len, "%s", xml_header, ctx->name, ctx->description); } else { - iio_snprintf(str, len, "%s", + ptr += iio_snprintf(str, len, "%s", xml_header, ctx->name); } - ptr = strrchr(str, '\0'); len = eptr - ptr; } @@ -122,8 +121,8 @@ char * iio_context_create_xml(const struct iio_context *ctx) } for (i = 0; i < ctx->nb_devices; i++) { - if (len > 0) { - iio_strlcpy(ptr, devices[i], len); + if (len > devices_len[i]) { + memcpy(ptr, devices[i], devices_len[i]); /* Flawfinder: ignore */ ptr += devices_len[i]; len -= devices_len[i]; } @@ -134,7 +133,7 @@ char * iio_context_create_xml(const struct iio_context *ctx) free(devices_len); if (len > 0) { - iio_strlcpy(ptr, "", len); + ptr += iio_strlcpy(ptr, "", len); len -= sizeof("") - 1; }