<div dir="ltr">Can I also suggest that we don't make this public API? These are internal helpers for libwayland, not designed for any consumers. We've been burned by making too much internal helper API public before. </div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Oct 27, 2014 at 2:42 AM, Marek Chalupa <<a href="mailto:mchqwerty@gmail.com" target="_blank">mchqwerty@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote"><div><div class="h5">On 22 October 2014 13:32, Imran Zaman <<a href="mailto:imran.zaman@gmail.com" target="_blank">imran.zaman@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">strtol/strtoul utility functions are used extensively in weston/wayland, and are not bug-free in their current form. To avoid definition in weston and wayland, its wrapped in functions with appropriate input and output checks. Test cases are also updated. Signed-off-by: Imran Zaman <<a href="mailto:imran.zaman@gmail.com" target="_blank">imran.zaman@gmail.com</a>> --- src/scanner.c | 5 +- src/wayland-client.c | 5 +- src/wayland-util.c | 55 ++++++++++++++++++++ src/wayland-util.h | 4 ++ tests/fixed-test.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 204 insertions(+), 7 deletions(-) diff --git a/src/scanner.c b/src/scanner.c index 809130b..165b12b 100644 --- a/src/scanner.c +++ b/src/scanner.c @@ -315,7 +315,6 @@ start_element(void *data, const char *element_name, const char **atts) struct description *description; const char *name, *type, *interface_name, *value, *summary, *since; const char *allow_null; - char *end; int i, version; ctx->loc.line_number = XML_GetCurrentLineNumber(ctx->parser); @@ -404,9 +403,7 @@ start_element(void *data, const char *element_name, const char **atts) message->destructor = 0; if (since != NULL) { - errno = 0; - version = strtol(since, &end, 0); - if (errno == EINVAL || end == since || *end != '\0') + if (!wl_strtol(since, NULL, 0, (long *)&version)) </blockquote><div> </div></div></div><div>This is baad. You cannot use the int version here, because in wl_strol you write sizeof(long) on the address </div><div>of version and if sizeof(version) is smaller than sizeof(long) then you overwrite memory. I'm getting ugly SIGSEGV </div><div>because of that. </div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> fail(&ctx->loc, "invalid integer (%s)\n", since); } else { diff --git a/src/wayland-client.c b/src/wayland-client.c index b0f77b9..fd98705 100644 --- a/src/wayland-client.c +++ b/src/wayland-client.c @@ -824,13 +824,12 @@ wl_display_connect_to_fd(int fd) WL_EXPORT struct wl_display * wl_display_connect(const char *name) { - char *connection, *end; + char *connection; int flags, fd; connection = getenv("WAYLAND_SOCKET"); if (connection) { - fd = strtol(connection, &end, 0); - if (*end != '\0') + if (!wl_strtol(connection, NULL, 0, (long *)&fd)) return NULL; flags = fcntl(fd, F_GETFD); diff --git a/src/wayland-util.c b/src/wayland-util.c index b099882..a930364 100644 --- a/src/wayland-util.c +++ b/src/wayland-util.c @@ -26,6 +26,8 @@ #include <stdio.h> #include <string.h> #include <stdarg.h> +#include <errno.h> +#include <ctype.h> #include "wayland-util.h" #include "wayland-private.h" @@ -361,6 +363,59 @@ wl_map_for_each(struct wl_map *map, wl_iterator_func_t func, void *data) for_each_helper(&map->server_entries, func, data); } +WL_EXPORT bool +wl_strtol(const char *str, char **endptr, int base, long *val) +{ + char *end = NULL; + long v; + + if (!str || !val) + return false; + if (!endptr) + endptr = &end; + + errno = 0; + v = strtol(str, endptr, base); + if (errno != 0 || *endptr == str || **endptr != '\0') + return false; + + *val = v; + return true; +} + +WL_EXPORT bool +wl_strtoul(const char *str, char **endptr, int base, unsigned long *val) +{ + char *end = NULL; + unsigned long v; + int i = 0; + + if (!str || !val) + return false; + + /* check for negative numbers */ + while (str[i]) { + if (!isspace(str[i])) { + if (str[i] == '-') + return false; + else + break; + } + i++; + } + + if (!endptr) + endptr = &end; + + errno = 0; + v = strtoul(str, endptr, base); + if (errno != 0 || *endptr == str || **endptr != '\0') + return false; + + *val = v; + return true; +} + static void wl_log_stderr_handler(const char *fmt, va_list arg) { diff --git a/src/wayland-util.h b/src/wayland-util.h index fd32826..c66cc41 100644 --- a/src/wayland-util.h +++ b/src/wayland-util.h @@ -36,6 +36,7 @@ extern "C" { #include <stddef.h> #include <inttypes.h> #include <stdarg.h> +#include <stdbool.h> /* GCC visibility */ #if defined(__GNUC__) && __GNUC__ >= 4 @@ -243,6 +244,9 @@ static inline wl_fixed_t wl_fixed_from_int(int i) return i * 256; } +bool wl_strtol(const char *str, char **endptr, int base, long *val); +bool wl_strtoul(const char *str, char **endptr, int base, unsigned long *val); + /** * \brief A union representing all of the basic data types that can be passed * along the wayland wire format. diff --git a/tests/fixed-test.c b/tests/fixed-test.c index 739a3b1..ad40467 100644 --- a/tests/fixed-test.c +++ b/tests/fixed-test.c @@ -88,3 +88,145 @@ TEST(fixed_int_conversions) i = wl_fixed_to_int(f); assert(i == -0x50); } + +TEST(strtol_conversions) +{ + bool ret; + long val = -1; + char *end = NULL, *str = NULL; + + ret = wl_strtol(NULL, NULL, 10, &val); + assert(ret == false); + assert(val == -1); + + ret = wl_strtol(NULL, NULL, 10, NULL); + assert(ret == false); + + str = "12"; + ret = wl_strtol(str, NULL, 10, &val); + assert(ret == true); + assert(val == 12); + + ret = wl_strtol(str, &end, 10, &val); + assert(end != NULL); + assert(*end == '\0'); + + str = "-12"; val = -1; + ret = wl_strtol(str, &end, 10, &val); + assert(ret == true); + assert(val == -12); + + str = "0x12"; val = -1; + ret = wl_strtol(str, &end, 16, &val); + assert(ret == true); + assert(val == 0x12); + + str = "A"; val = -1; + ret = wl_strtol(str, &end, 16, &val); + assert(ret == true); + assert(val == 10); + + str = "-0x20"; val = -1; + ret = wl_strtol(str, &end, 16, &val); + assert(ret == true); + assert(val == -0x20); + + str = "0012"; val = -1; + ret = wl_strtol(str, &end, 8, &val); + assert(ret == true); + assert(val == 10); + + str = "0101"; val = -1; + ret = wl_strtol(str, &end, 2, &val); + assert(ret == true); + assert(val == 5); + + str = "s12"; val = -1; + ret = wl_strtol(str, NULL, 10, &val); + assert(ret == false); + assert(val == -1); + + ret = wl_strtol(str, &end, 10, &val); + assert(end == str); + + str = "214748364789L"; val = -1; + ret = wl_strtol(str, NULL, 10, &val); + assert(ret == false); + assert(val == -1); + + str = ""; val = -1; + ret = wl_strtol(str, NULL, 10, &val); + assert(ret == false); + assert(val == -1); +} + +TEST(strtoul_conversions) +{ + bool ret; + unsigned long val = 0; + char *end = NULL, *str = NULL; + + ret = wl_strtoul(NULL, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + ret = wl_strtoul(NULL, NULL, 10, NULL); + assert(ret == false); + + str = "15"; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == true); + assert(val == 15); + + ret = wl_strtoul(str, &end, 10, &val); + assert(end != NULL); + assert(*end == '\0'); + + str = "0x12"; val = 0; + ret = wl_strtoul(str, &end, 16, &val); + assert(ret == true); + assert(val == 18); + + str = "A"; val = 0; + ret = wl_strtoul(str, &end, 16, &val); + assert(ret == true); + assert(val == 10); + + str = "0012"; val = 0; + ret = wl_strtoul(str, &end, 8, &val); + assert(ret == true); + assert(val == 10); + + str = "0101"; val = 0; + ret = wl_strtoul(str, &end, 2, &val); + assert(ret == true); + assert(val == 5); + + str = "s15"; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + ret = wl_strtoul(str, &end, 10, &val); + assert(end == str); + + str = "429496729533UL"; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + str = "-1"; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + str = " -1234"; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + str = ""; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); +} </blockquote><div> </div></div></div><div>Maybe the tests are big enough to deserve it's own binary? fixed-tests has nothing in common with wl_strtol. My opinion only.. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> -- 1.9.1 _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org" target="_blank">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote><div> </div><div>Cheers, </div><div>Marek </div></div> </div></div> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div> -- Jasper </div>