diff mbox

[Xen-devel] tools: libxl: do not overrun input buffer in libxl__parse_mac

Message ID 1415282383-26594-1-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell Nov. 6, 2014, 1:59 p.m. UTC 
Valgrind reports:
==7971== Invalid read of size 1
==7971==    at 0x40877BE: libxl__parse_mac (libxl_internal.c:288)
==7971==    by 0x405C5F8: libxl__device_nic_from_xs_be (libxl.c:3405)
==7971==    by 0x4065542: libxl__append_nic_list_of_type (libxl.c:3484)
==7971==    by 0x4065542: libxl_device_nic_list (libxl.c:3504)
==7971==    by 0x406F561: libxl_retrieve_domain_configuration (libxl.c:6661)
==7971==    by 0x805671C: reload_domain_config (xl_cmdimpl.c:2037)
==7971==    by 0x8057F30: handle_domain_death (xl_cmdimpl.c:2116)
==7971==    by 0x8057F30: create_domain (xl_cmdimpl.c:2580)
==7971==    by 0x805B4B2: main_create (xl_cmdimpl.c:4652)
==7971==    by 0x804EAB2: main (xl.c:378)

This is because on the final iteration the tok += 3 skips over the terminating
NUL to the next byte, and then *tok reads it. Fix this by using endptr as the
iterator.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_internal.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Wei Liu Nov. 6, 2014, 2:55 p.m. UTC | #1 
On Thu, Nov 06, 2014 at 01:59:43PM +0000, Ian Campbell wrote:
> Valgrind reports:
> ==7971== Invalid read of size 1
> ==7971==    at 0x40877BE: libxl__parse_mac (libxl_internal.c:288)
> ==7971==    by 0x405C5F8: libxl__device_nic_from_xs_be (libxl.c:3405)
> ==7971==    by 0x4065542: libxl__append_nic_list_of_type (libxl.c:3484)
> ==7971==    by 0x4065542: libxl_device_nic_list (libxl.c:3504)
> ==7971==    by 0x406F561: libxl_retrieve_domain_configuration (libxl.c:6661)
> ==7971==    by 0x805671C: reload_domain_config (xl_cmdimpl.c:2037)
> ==7971==    by 0x8057F30: handle_domain_death (xl_cmdimpl.c:2116)
> ==7971==    by 0x8057F30: create_domain (xl_cmdimpl.c:2580)
> ==7971==    by 0x805B4B2: main_create (xl_cmdimpl.c:4652)
> ==7971==    by 0x804EAB2: main (xl.c:378)
> 
> This is because on the final iteration the tok += 3 skips over the terminating
> NUL to the next byte, and then *tok reads it. Fix this by using endptr as the
> iterator.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

This is a candidate for backporting.

Wei.
Ian Jackson Nov. 6, 2014, 4:35 p.m. UTC | #2 
Wei Liu writes ("Re: [PATCH] tools: libxl: do not overrun input buffer in libx\
l__parse_mac"):
> > This is because on the final iteration the tok += 3 skips over the termina\
ting
> > NUL to the next byte, and then *tok reads it. Fix this by using endptr as \
the
> > iterator.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

> This is a candidate for backporting.

Queued.

Ian, I don't suppose you could persuade your text editor to wrap your
commit messages a bit narrower ?  By the time they've been quoted a
couple of times they look to me like what you see above.

Thanks,
Ian.
diff mbox

Patch

diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index 02a71cb..00c3b1e 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -284,10 +284,12 @@  _hidden int libxl__parse_mac(const char *s, libxl_mac mac)
     char *endptr;
     int i;
 
-    for (i = 0, tok = s; *tok && (i < 6); ++i, tok += 3) {
+    for (i = 0, tok = s; *tok && (i < 6); ++i, tok = endptr) {
         mac[i] = strtol(tok, &endptr, 16);
         if (endptr != (tok + 2) || (*endptr != '\0' && *endptr != ':') )
             return ERROR_INVAL;
+        if (*endptr == ':')
+            endptr++;
     }
     if ( i != 6 )
         return ERROR_INVAL;