One call to va_end() was missing on an error path. --- ell/asn1-private.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ell/asn1-private.h b/ell/asn1-private.h index 7827355..4ba17ac 100644 --- a/ell/asn1-private.h +++ b/ell/asn1-private.h @@ -108,8 +108,10 @@ static inline const uint8_t *asn1_der_find_elem_by_path(const uint8_t *buf, pos = va_arg(vl, int); - if (!buf || elem_tag != (pos == -1 ? tag : ASN1_ID_SEQUENCE)) + if (!buf || elem_tag != (pos == -1 ? tag : ASN1_ID_SEQUENCE)) { + va_end(vl); return NULL; + } } va_end(vl); -- 2.14.1

--- ell/util.c | 32 ++++++++++++++++++++++++++++++++ ell/util.h | 2 ++ 2 files changed, 34 insertions(+) diff --git a/ell/util.c b/ell/util.c index 98916e5..2879f9f 100644 --- a/ell/util.c +++ b/ell/util.c @@ -443,6 +443,38 @@ LIB_EXPORT bool l_str_has_prefix(const char *str, const char *prefix) return !strncmp(str, prefix, prefix_len); } +/** + * l_strlcpy: + * @dst: Destination buffer for string + * @src: Source buffer containing null-terminated string to copy + * @len: Maximum destination buffer space to use + * + * Copies a string from the @src buffer to the @dst buffer, using no + * more than @len bytes in @dst. @dst is guaranteed to be + * null-terminated. The caller can determine if the copy was truncated by + * checking if the return value is greater than or equal to @len. + * + * Returns: The length of the @src string, not including the null + * terminator. + */ +LIB_EXPORT size_t l_strlcpy(char* dst, const char *src, size_t len) +{ + size_t src_len = strlen(src); + + if (len) { + if (src_len < len) { + len = src_len + 1; + } else { + len -= 1; + dst[len] = '\0'; + } + + memcpy(dst, src, len); + } + + return src_len; +} + /** * l_str_has_suffix: * @str: A string to be examined diff --git a/ell/util.h b/ell/util.h index a77c0b8..126a172 100644 --- a/ell/util.h +++ b/ell/util.h @@ -222,6 +222,8 @@ char *l_strjoinv(char **str_array, const char delim); bool l_str_has_prefix(const char *str, const char *prefix); bool l_str_has_suffix(const char *str, const char *suffix); +size_t l_strlcpy(char* dst, const char *src, size_t len); + char *l_util_hexstring(const unsigned char *buf, size_t len); unsigned char *l_util_from_hexstring(const char *str, size_t *out_len); -- 2.14.1

Hi Denis, On Mon, 25 Sep 2017, Denis Kenzior wrote: It's about twice as fast to use strlen/memcpy. The direct implementation I used for comparison: const char *orig_src = src; if (len) { while (*src != '\0' && --len) { *dst++ = *src++; } *dst = '\0'; } return (src - orig_src) + strlen(src); I modified the unit test to run a bunch of iterations with some more string sizes and to take performance samples during the strlcpy test. gperftools showed that the direct code spent 57.39% of execution time inside l_strlcpy and the strlen/memcpy technique spent 16.06% of execution time inside l_strlcpy. Turning off the profiling stuff and just using 'time', direct took 53.973 seconds and strlen/memcpy took 25.176 seconds of user time. -- Mat Martineau Intel OTC

--- unit/test-util.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/unit/test-util.c b/unit/test-util.c index 427c74a..d2b5496 100644 --- a/unit/test-util.c +++ b/unit/test-util.c @@ -74,6 +74,45 @@ static void test_has_suffix(const void *test_data) assert(!l_str_has_suffix(suffix, str)); } +static void do_strlcpy(size_t dst_bytes, size_t src_bytes) +{ + /* A valid address is needed for the destination buffer + * even if l_strlcpy is told to not copy anything there + */ + char *dst = l_malloc(dst_bytes ?: 1); + char *src = l_malloc(src_bytes); + size_t src_strlen = src_bytes - 1; + + memset(dst, ' ', dst_bytes ?: 1); + memset(src, '@', src_strlen); + src[src_strlen] = '\0'; + + assert(l_strlcpy(dst, src, dst_bytes) == src_strlen); + + if (!dst_bytes) { + assert(*dst == ' '); + } else if (src_strlen >= dst_bytes) { + /* Copy was truncated */ + assert(strlen(dst) == dst_bytes - 1); + assert(l_str_has_prefix(src, dst)); + } else + assert(!strcmp(src, dst)); + + l_free(dst); + l_free(src); +} + +static void test_strlcpy(const void *test_data) +{ + do_strlcpy(0, 1); + do_strlcpy(0, 10); + do_strlcpy(10, 8); + do_strlcpy(10, 9); + do_strlcpy(10, 10); + do_strlcpy(10, 11); + do_strlcpy(10, 12); +} + int main(int argc, char *argv[]) { l_test_init(&argc, &argv); @@ -83,5 +122,7 @@ int main(int argc, char *argv[]) l_test_add("l_util_has_suffix", test_has_suffix, NULL); + l_test_add("l_strlcpy", test_strlcpy, NULL); + return l_test_run(); } -- 2.14.1

Hi Mat, On 09/22/2017 05:12 PM, Mat Martineau wrote: It would seem that l_genl_family_new should sanitize the name. There's no point in truncating a name silently and trying to use it afterwards just to fail spectacularly anyway. The other two are not called with user input but get their values from the kernel, no? Regards, -Denis

Hi Mat, But then we're allocating a family object, memsetting it, etc unnecessarily, no? why is it incorrect? We're getting the name from the kernel and copying it into our own data structure. If the kernel uses GENL_NAMSIZ max-size buffers, then using strncpy is just fine, no? At least I don't see how strlcpy is saving you here. If you want to be paranoid, you'd be taking the tlv length value into account as well. Regards, -Denis