[PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions
Giulio Camuffo
giuliocamuffo at gmail.com
Tue Oct 28 04:21:14 PDT 2014
2014-10-27 18:51 GMT+02:00 Jasper St. Pierre <jstpierre at mecheye.net>:
> 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 <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
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list