I pushed an updated patch v3. Added test cases for overflow and also check for negative numbers for wl_strtoul.. please review
BR imran On Wed, Oct 22, 2014 at 12:13 PM, Marek Chalupa <mchqwe...@gmail.com> wrote: > Hi, > > 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.. > Anyway, I have few nitpicks and a questions - see below. > > On 16 October 2014 18:11, Imran Zaman <imran.za...@gmail.com> wrote: >> >> 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 <imran.za...@gmail.com> >> --- >> 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; > > > The 'return false' and 'endptr =&end' should be on new line > http://cgit.freedesktop.org/wayland/wayland/tree/doc/Contributing#n33 > >> >> + >> + 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; > > > The same > >> >> + >> + 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"; > > > This variable should be declared on the beginning of the function and only > assigned here > (again from the Contributing document). > >> >> + 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"; > > > the same > >> >> + 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 > > > 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. > > Regards, > Marek > >> >> >> _______________________________________________ >> wayland-devel mailing list >> wayland-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel