2014-10-27 18:51 GMT+02:00 Jasper St. Pierre <[email protected]>: > 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.
+1 I don't think this belongs in the wayland API at all. That means duplicating them in weston, but they will hardly need modifications anyway. -- Giulio > > On Mon, Oct 27, 2014 at 2:42 AM, Marek Chalupa <[email protected]> wrote: >> >> >> >> On 22 October 2014 13:32, Imran Zaman <[email protected]> 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 <[email protected]> >>> --- >>> 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)) >> >> >> This is baad. You cannot use the int version here, because in wl_strol you >> write sizeof(long) on the address >> of version and if sizeof(version) is smaller than sizeof(long) then you >> overwrite memory. I'm getting ugly SIGSEGV >> because of that. >> >> >>> >>> 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); >>> +} >> >> >> Maybe the tests are big enough to deserve it's own binary? fixed-tests has >> nothing in common with wl_strtol. >> My opinion only.. >> >>> >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> wayland-devel mailing list >>> [email protected] >>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel >> >> >> Cheers, >> Marek >> >> >> _______________________________________________ >> wayland-devel mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel >> > > > > -- > Jasper > > _______________________________________________ > wayland-devel mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
