<div dir="ltr"><div>Hi, </div><div>Personally, I'd rather see these functions private (somebody has already mentioned that), but I understand there are reasons for them to be public and maybe in the end it will have more pros.. </div><div>Anyway, I have few nitpicks and a questions - see below. </div><div class="gmail_extra"> <div class="gmail_quote">On 16 October 2014 18:11, 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">imran.zaman@gmail.com</a>> --- src/scanner.c | 5 +-- src/wayland-client.c | 5 +-- src/wayland-util.c | 38 ++++++++++++++++ src/wayland-util.h | 4 ++ tests/fixed-test.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 167 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)) 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..dfd2eb1 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 <limits.h> #include "wayland-util.h" #include "wayland-private.h" @@ -361,6 +363,42 @@ 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; </blockquote><div> </div><div>The 'return false' and 'endptr =&end' should be on new line <a href="http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33">http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33</a> </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"> + + 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; + + if (!str || !val) return false; + if (!endptr) endptr = &end; </blockquote><div> </div><div>The same </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"> + + 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..aa0340c 100644 --- a/tests/fixed-test.c +++ b/tests/fixed-test.c @@ -88,3 +88,125 @@ TEST(fixed_int_conversions) i = wl_fixed_to_int(f); assert(i == -0x50); } + +TEST(strtol_conversions) +{ + bool ret; + long val = -1; + char *end = NULL; + + ret = wl_strtol(NULL, NULL, 10, &val); + assert(ret == false); + assert(val == -1); + + ret = wl_strtol(NULL, NULL, 10, NULL); + assert(ret == false); + + char *str = "12"; </blockquote><div> </div><div>This variable should be declared on the beginning of the function and only assigned here (again from the Contributing document). </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"> + 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 = ""; 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; + + ret = wl_strtoul(NULL, NULL, 10, &val); + assert(ret == false); + assert(val == 0); + + ret = wl_strtoul(NULL, NULL, 10, NULL); + assert(ret == false); + + char *str = "15"; </blockquote><div> </div><div>the same </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"> + 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 = ""; val = 0; + ret = wl_strtoul(str, NULL, 10, &val); + assert(ret == false); + assert(val == 0); +} -- 1.9.1 </blockquote><div> </div><div>What I'm wondering is what should be the behavior of wl_strtoul for negative numbers? strtoul silently converts them, but I don't think this is what we always want... or is it? And also some tests for overflow would be handy. </div><div>Regards, Marek </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"> _______________________________________________ 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> </div></div>