[PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
Jasper St. Pierre
jstpierre at mecheye.net
Mon Oct 27 09:51:51 PDT 2014
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.
On Mon, Oct 27, 2014 at 2:42 AM, Marek Chalupa <mchqwerty at gmail.com> wrote:
>
>
> On 22 October 2014 13:32, Imran Zaman <imran.zaman at 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.zaman at gmail.com>
>> ---
>> 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
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
> Cheers,
> Marek
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
--
Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141027/6d8d0d92/attachment-0001.html>
More information about the wayland-devel
mailing list