[PATCH] mdev regex fix

[Natanael Copa]

On Fri, 2008-05-23 at 22:51 +0200, Denys Vlasenko wrote:
> On Friday 23 May 2008 14:36, Natanael Copa wrote:
> > forgot to attach the patch.
> > sorry
> > 
> > On Fri, 2008-05-23 at 14:30 +0200, Natanael Copa wrote:
> > > mdev in busybox-1.10.2 has a bug:
> > > 
> > > mdev.conf:
> > > zap(.+)		root:dialout 0660 =zap/%1
> > > 
> > > When expanding the %1 regex, string will not be terminated with a '\0'
> > > properly causing interesting results in /dev. 
> > > 
> > > # ls /dev/zap/
> > > channel  ctlS
    ctlT
    pseudo   timer
> > > 
> > > 
> > > Attatched patch fixes the issue. (for 1.10.2 + current patches)
> 
> Strange. It means that we allocate too small an alias,
> because otherwise it would be NUL terminated because of xzalloc!

You are absolutely right. Didn't think of that.

> 
>                                 /* substitute %1..9 with off[1..9], if any */
>                                 n = 0;
>                                 s = val;

Problem is in the line below here (yeah, i got tricked by it too and
assumed that the buf was big enough). It does not do what you think it
does:

>                                 while (*s && *s++ == '%')
>                                         n++;

Should be:

	while (*s)
		if (*s++ == '%')
			n++;


> 
> =======>                        p = alias = xzalloc(strlen(val) + n * strlen(device_name));
>                                 s = val + 1;
>                                 while (*s) {
>                                         *p = *s;
>                                         if ('%' == *s) {
>                                                 i = (s[1] - '0');
>                                                 if (i <= 9 && off[i].rm_so >= 0) {
>                                                         n = off[i].rm_eo - off[i].rm_so;
>                                                         strncpy(p, device_name + off[i].rm_so, n);
>                                                         p += n - 1;
>                                                         s++;
>                                                 }
>                                         }
>                                         p++;
>                                         s++;
>                                 }
> 
> The replacement string starts at val+1. If val has no '%', its length is
> strlen(val+1), with terminating NUL it is strlen(val+1) + 1 == strlen(val).
> 
> Each '%' is substituted by device_name at max (meaning: it can be substituted
> by just a part of device_name, or the whole device_name). We have n '%' chars,
> so we need at most strlen(val) + n * strlen(device_name) chars, including NUL.
> 
> In your example:
> 
> mdev.conf:
> zap(.+)         root:dialout 0660 =zap/%1
> 
> strlen(val) + n * strlen(device_name) == 7 + strlen(device_name).
> If device_name == "zap.ctlT", it's 7+8 = 15, and it's enough for "zap/ctlT".
> 
> I am obviously wrong somewhere, but I don't see where.

The problem was that n == 0.
If device_name == "zapctl" and n == 0 the buf size will end up as 7,
which is 1 too small for "zap/ctl".

> --
> vda

Attatched is updated patch.

-nc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-1.10.2-mdev-regex-fix2.patch
Type: text/x-patch
Size: 386 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20080524/b8642cfe/attachment-0002.bin