wl_strtol and wl_strtoul utility functions are added

Submitted by Imran Zaman on Oct. 15, 2014, 1:14 p.m.

Details

Message ID CAPfuZRjs+edcM4_J-6wNMKpwaH3KuJb+U2QZPE9ZzfbdeZE3GA@mail.gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Imran Zaman Oct. 15, 2014, 1:14 p.m.
Hi

The patch is used to replace strtol and strtoul with wl_strtol and
wl_strtoul with inputs and result checks.

The utility functions are used extensively in wayland and weston so added
appropriate
input and output checks; test cases are also updated; will push the patch
for weston as well.

BR
imran

Patch hide | download patch | download mbox

commit 6be79948c9d408bb4f61cec5fff391f7ed7beb7b
Author: Imran Zaman <imran.zaman@intel.com>
Date:   Wed Oct 15 16:02:16 2014 +0300

    wayland-util: added wl_strtol/wl_strtoul utility functions
    
    The utility functions are used extensively so added appropriate
    checks and test cases.
    
    Signed-off-by: Imran Zaman <imran.zaman@intel.com>

diff --git a/src/scanner.c b/src/scanner.c
index 809130b..3e30fe7 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, &version))
 				fail(&ctx->loc,
 				     "invalid integer (%s)\n", since);
 		} else {
diff --git a/src/wayland-client.c b/src/wayland-client.c
index b0f77b9..1229b5f 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, &fd))
 			return NULL;
 
 		flags = fcntl(fd, F_GETFD);
diff --git a/src/wayland-util.c b/src/wayland-util.c
index b099882..f8267f3 100644
--- a/src/wayland-util.c
+++ b/src/wayland-util.c
@@ -26,6 +26,9 @@ 
 #include <stdio.h>
 #include <string.h>
 #include <stdarg.h>
+#include <errno.h>
+#include <limits.h>
+#include <stdlib.h>
 
 #include "wayland-util.h"
 #include "wayland-private.h"
@@ -361,6 +364,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 int
+wl_strtol(const char *str, char **endptr, int base, int32_t *val)
+{
+	char *end = NULL;
+	long v;
+
+	if (!str || !val) return 0;
+	if (!endptr) endptr = &end;
+
+	errno = 0;
+	v = strtol(str, endptr, base);
+	if (errno != 0 || *endptr == str || **endptr != '\0')
+		return 0;
+
+	*val = v;
+	return 1;
+}
+
+WL_EXPORT int
+wl_strtoul(const char *str, char **endptr, int base, uint32_t *val)
+{
+	char *end = NULL;
+	unsigned long v;
+
+	if (!str || !val) return 0;
+	if (!endptr) endptr = &end;
+
+	errno = 0;
+	v = strtoul(str, endptr, base);
+	if (errno != 0 || *endptr == str || **endptr != '\0')
+		return 0;
+
+	*val = v;
+	return 1;
+}
+
 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..b77d4e3 100644
--- a/src/wayland-util.h
+++ b/src/wayland-util.h
@@ -243,6 +243,9 @@  static inline wl_fixed_t wl_fixed_from_int(int i)
 	return i * 256;
 }
 
+int wl_strtol(const char *str, char **endptr, int base, int32_t *val);
+int wl_strtoul(const char *str, char **endptr, int base, uint32_t *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..349cc48 100644
--- a/tests/fixed-test.c
+++ b/tests/fixed-test.c
@@ -88,3 +88,61 @@  TEST(fixed_int_conversions)
 	i = wl_fixed_to_int(f);
 	assert(i == -0x50);
 }
+
+TEST(strtol_conversions)
+{
+	int ret;
+	int32_t val = -1;
+	char *end = NULL;
+
+	char *str = "12";
+	ret = wl_strtol(str, NULL, 10, &val);
+	assert(ret == 1);
+	assert(val == 12);
+
+	ret = wl_strtol(str, &end, 10, &val);
+	assert(end != NULL);
+	assert(*end == '\0');
+
+	str = "s12"; val = -1;
+	ret = wl_strtol(str, NULL, 10, &val);
+	assert(ret == 0);
+	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 == 0);
+	assert(val == -1);
+}
+
+TEST(strtoul_conversions)
+{
+	int ret;
+	uint32_t val = 0;
+	char *end = NULL;
+
+	char *str = "15";
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == 1);
+	assert(val == 15);
+
+	ret = wl_strtoul(str, &end, 10, &val);
+	assert(end != NULL);
+	assert(*end == '\0');
+
+	str = "s15"; val = 0;
+	ret = wl_strtoul(str, NULL, 10, &val);
+	assert(ret == 0);
+	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 == 0);
+	assert(val == 0);
+}

Comments

Le 2014-10-15 16:14, Imran Zaman a écrit :
> Hi
>
> The patch is used to replace strtol and strtoul with wl_strtol and
> wl_strtoul with inputs and result checks.

I don't know where Wayland developers stand on this, but I would rather 
the client library function calls not clobber errno to zero.
I don't see how this belongs in libwayland.  Sure, we use strtol twice, but
I don't think that warrants adding 100 lines of wrapper functions and test
cases.
--Jason Ekstrand

On Wed, Oct 15, 2014 at 6:16 AM, Rémi Denis-Courmont <remi@remlab.net>
wrote:

> Le 2014-10-15 16:14, Imran Zaman a écrit :
>
>> Hi
>>
>> The patch is used to replace strtol and strtoul with wl_strtol and
>> wl_strtoul with inputs and result checks.
>>
>
> I don't know where Wayland developers stand on this, but I would rather
> the client library function calls not clobber errno to zero.
>
> --
> Rémi Denis-Courmont
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
The reason is that strtol is used at many places in weston/wayland..
and its not covering all the error cases everywhere (i.e. its buggy)..
so its better to
encapsulate it in a function with all the input and output checks...
it can be moved to weston if its sound such a big deal...

On Wed, Oct 15, 2014 at 9:42 PM, Jason Ekstrand <jason@jlekstrand.net> wrote:
> I don't see how this belongs in libwayland.  Sure, we use strtol twice, but
> I don't think that warrants adding 100 lines of wrapper functions and test
> cases.
> --Jason Ekstrand
>
> On Wed, Oct 15, 2014 at 6:16 AM, Rémi Denis-Courmont <remi@remlab.net>
> wrote:
>>
>> Le 2014-10-15 16:14, Imran Zaman a écrit :
>>>
>>> Hi
>>>
>>> The patch is used to replace strtol and strtoul with wl_strtol and
>>> wl_strtoul with inputs and result checks.
>>
>>
>> I don't know where Wayland developers stand on this, but I would rather
>> the client library function calls not clobber errno to zero.
>>
>>
>> --
>> Rémi Denis-Courmont
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
According the the Linux man page you have to set errno to zero to use 
strtol correctly anyway.

Quick test shows this is the only way to detect out-of-range errors. The 
glib strtol consumes all digits no matter how many you type in (rather 
than possibly quitting on the first digit that makes the number go out 
of range, which would all this to be detected by looking at the end 
character).

It seems the only way to fix this would be to write the entire 
conversion without using strtol. That might be getting out of what 
Wayland wants to write however.

On 10/15/2014 11:42 AM, Jason Ekstrand wrote:
> I don't see how this belongs in libwayland.  Sure, we use strtol twice,
> but I don't think that warrants adding 100 lines of wrapper functions
> and test cases.
> --Jason Ekstrand
>
> On Wed, Oct 15, 2014 at 6:16 AM, Rémi Denis-Courmont <remi@remlab.net
> <mailto:remi@remlab.net>> wrote:
>
>     Le 2014-10-15 16:14, Imran Zaman a écrit :
>
>         Hi
>
>         The patch is used to replace strtol and strtoul with wl_strtol and
>         wl_strtoul with inputs and result checks.
>
>
>     I don't know where Wayland developers stand on this, but I would
>     rather the client library function calls not clobber errno to zero.
>
>     --
>     Rémi Denis-Courmont
>     _________________________________________________
>     wayland-devel mailing list
>     wayland-devel@lists.__freedesktop.org
>     <mailto:wayland-devel@lists.freedesktop.org>
>     http://lists.freedesktop.org/__mailman/listinfo/wayland-devel
>     <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
>
On Wednesday 15 October 2014 16:16:34 Rémi Denis-Courmont wrote:
> Le 2014-10-15 16:14, Imran Zaman a écrit :
> > Hi
> > 
> > The patch is used to replace strtol and strtoul with wl_strtol and
> > wl_strtoul with inputs and result checks.
> 
> I don't know where Wayland developers stand on this, but I would rather
> the client library function calls not clobber errno to zero.

There's no other way to detect strto(u)l errors. It returns either 0, 
LONG_MIN, LONG_MAX or ULONG_MAX for errors, but those are also valid values.
On Wednesday 15 October 2014 16:14:17 Imran Zaman wrote:
> Hi
> 
> The patch is used to replace strtol and strtoul with wl_strtol and
> wl_strtoul with inputs and result checks.
> 
> The utility functions are used extensively in wayland and weston so added
> appropriate
> input and output checks; test cases are also updated; will push the patch
> for weston as well.

You should use strtol_l and strtoul_l from <xlocale.h> and pass the C locale.

Otherwise, those functions are subject to the locale's definition of a space 
character.
On Wed, 15 Oct 2014 22:04:46 +0300
Imran Zaman <imran.zaman@gmail.com> wrote:

> The reason is that strtol is used at many places in weston/wayland..
> and its not covering all the error cases everywhere (i.e. its buggy)..
> so its better to
> encapsulate it in a function with all the input and output checks...
> it can be moved to weston if its sound such a big deal...

libwayland-* are not generic utility libraries. We will not export this
kind of functions, even if it might seem useful on a first glance.
Libwayland is not a bucket where you throw everything you can imagine
to be useful also elsewhere.

Internal helpers would be a different matter.

> On Wed, Oct 15, 2014 at 9:42 PM, Jason Ekstrand <jason@jlekstrand.net> wrote:
> > I don't see how this belongs in libwayland.  Sure, we use strtol twice, but
> > I don't think that warrants adding 100 lines of wrapper functions and test
> > cases.
> > --Jason Ekstrand
> >
> > On Wed, Oct 15, 2014 at 6:16 AM, Rémi Denis-Courmont <remi@remlab.net>
> > wrote:
> >>
> >> Le 2014-10-15 16:14, Imran Zaman a écrit :
> >>>
> >>> Hi
> >>>
> >>> The patch is used to replace strtol and strtoul with wl_strtol and
> >>> wl_strtoul with inputs and result checks.
> >>
> >>
> >> I don't know where Wayland developers stand on this, but I would rather
> >> the client library function calls not clobber errno to zero.

Yeah, that's probably a good rule.


Thanks,
pq
I have already taken off that patch from the patchwork list...

and pushed a simpler version...
http://lists.freedesktop.org/archives/wayland-devel/2014-November/018030.html

BR
imran

On Wed, Nov 5, 2014 at 5:08 PM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Wed, 15 Oct 2014 22:04:46 +0300
> Imran Zaman <imran.zaman@gmail.com> wrote:
>
> > The reason is that strtol is used at many places in weston/wayland..
> > and its not covering all the error cases everywhere (i.e. its buggy)..
> > so its better to
> > encapsulate it in a function with all the input and output checks...
> > it can be moved to weston if its sound such a big deal...
>
> libwayland-* are not generic utility libraries. We will not export this
> kind of functions, even if it might seem useful on a first glance.
> Libwayland is not a bucket where you throw everything you can imagine
> to be useful also elsewhere.
>
> Internal helpers would be a different matter.
>
> > On Wed, Oct 15, 2014 at 9:42 PM, Jason Ekstrand <jason@jlekstrand.net>
> wrote:
> > > I don't see how this belongs in libwayland.  Sure, we use strtol
> twice, but
> > > I don't think that warrants adding 100 lines of wrapper functions and
> test
> > > cases.
> > > --Jason Ekstrand
> > >
> > > On Wed, Oct 15, 2014 at 6:16 AM, Rémi Denis-Courmont <remi@remlab.net>
> > > wrote:
> > >>
> > >> Le 2014-10-15 16:14, Imran Zaman a écrit :
> > >>>
> > >>> Hi
> > >>>
> > >>> The patch is used to replace strtol and strtoul with wl_strtol and
> > >>> wl_strtoul with inputs and result checks.
> > >>
> > >>
> > >> I don't know where Wayland developers stand on this, but I would
> rather
> > >> the client library function calls not clobber errno to zero.
>
> Yeah, that's probably a good rule.
>
>
> Thanks,
> pq
>