Patchwork [1/1] busybox: Add upstream patch to avoid occasional mdev SIGSEGV.

login
register
mail settings
Submitter Raúl Sánchez Siles
Date June 11, 2013, 4:24 p.m.
Message ID <20130611162412.GE18264@trismegisto.universo>
Download mbox | patch
Permalink /patch/250571/
State Not Applicable
Headers show

Comments

Raúl Sánchez Siles - June 11, 2013, 4:24 p.m.
This implies rename of previous patch for a correct patch ordering.

Signed-off-by: Raúl Sánchez Siles <rasasi78@gmail.com>
---
 ...21.0-mdev.patch => busybox-1.21.0-mdev_1.patch} |    0
 .../busybox-1.21.0-mdev_2_check_ACTION.patch       |   32 ++++++++++++++++++++
 2 files changed, 32 insertions(+)
 rename package/busybox/1.21.0/{busybox-1.21.0-mdev.patch => busybox-1.21.0-mdev_1.patch} (100%)
 create mode 100644 package/busybox/1.21.0/busybox-1.21.0-mdev_2_check_ACTION.patch
Peter Korsgaard - June 11, 2013, 8:59 p.m.
>>>>> "Raúl" == Raúl Sánchez Siles <rasasi78@gmail.com> writes:

 Raúl> This implies rename of previous patch for a correct patch ordering.
 Raúl> Signed-off-by: Raúl Sánchez Siles <rasasi78@gmail.com>
 Raúl> ---
 Raúl>  ...21.0-mdev.patch => busybox-1.21.0-mdev_1.patch} |    0
 Raúl>  .../busybox-1.21.0-mdev_2_check_ACTION.patch       |   32 ++++++++++++++++++++

I would prefer to not rename the existing patch, as it makes it less
clear where it comes from (http://busybox.net/downloads/fixes-1.21.0/).

It shouldn't be necessary either as '.' comes before '_' (but not before
'-').

Talking of which, why is this patch not in the fixes directory? Does
Denys not consider it important enough?
Raúl Sánchez Siles - June 12, 2013, 6:24 a.m.
Hi:

El Martes, 11 de junio de 2013, Peter Korsgaard escribió:
> >>>>> "Raúl" == Raúl Sánchez Siles <rasasi78@gmail.com> writes:
>  Raúl> This implies rename of previous patch for a correct patch ordering.
>  Raúl> Signed-off-by: Raúl Sánchez Siles <rasasi78@gmail.com>
>  Raúl> ---
>  Raúl>  ...21.0-mdev.patch => busybox-1.21.0-mdev_1.patch} |    0
>  Raúl>  .../busybox-1.21.0-mdev_2_check_ACTION.patch       |   32
> ++++++++++++++++++++
> 
> I would prefer to not rename the existing patch, as it makes it less
> clear where it comes from (http://busybox.net/downloads/fixes-1.21.0/).
> 
> It shouldn't be necessary either as '.' comes before '_' (but not before
> '-').

  That's not what I got here. I've done a more comprehensive test:
$ ls -1 |sort 
busybox-1.21.0-mdev_2_check_ACTION.patch
busybox-1.21.0-mdev_check_ACTION.patch
busybox-1.21.0-mdev-check_ACTION.patch
busybox-1.21.0-mdev.patch

  As you can see the one we want to apply first comes last compared with other 
possibilities :/

> 
> Talking of which, why is this patch not in the fixes directory? Does
> Denys not consider it important enough?

  That's probably something to ask him. I found this bug by chance and I don't 
consider it critical in any way, altough I haven't explored further 
implications. This can be reproduced when you invoke mdev without parameters. It 
will SIGSEGV since, as per current 1.21.0 code, you'll check for ACTION 
environment variable, which doesn't exist. This means a null pointer which is 
passed to index_in_strings which, in turn, passes it to the uclibc strcmp 
function that is unable to handle this case gracefully.

  Regards,
Peter Korsgaard - June 12, 2013, 8:54 p.m.
>>>>> "Raúl" == Raúl Sánchez Siles <rasasi78@gmail.com> writes:

Hi,

 >> I would prefer to not rename the existing patch, as it makes it less
 >> clear where it comes from (http://busybox.net/downloads/fixes-1.21.0/).
 >> 
 >> It shouldn't be necessary either as '.' comes before '_' (but not before
 >> '-').

 Raúl>   That's not what I got here. I've done a more comprehensive test:
 Raúl> $ ls -1 |sort 
 Raúl> busybox-1.21.0-mdev_2_check_ACTION.patch
 Raúl> busybox-1.21.0-mdev_check_ACTION.patch
 Raúl> busybox-1.21.0-mdev-check_ACTION.patch
 Raúl> busybox-1.21.0-mdev.patch

 Raúl>   As you can see the one we want to apply first comes last
 Raúl> compared with other possibilities :/

That's because localization is odd.

touch busybox-1.21.0-mdev_2_check_ACTION.patch busybox-1.21.0-mdev.patch

ls -1
busybox-1.21.0-mdev_2_check_ACTION.patch
busybox-1.21.0-mdev.patch

LANG=C ls -1
busybox-1.21.0-mdev.patch
busybox-1.21.0-mdev_2_check_ACTION.patch

We should arguably force LANG=C (or atleast LC_COLLATE, E.G. the sort
order) in apply-patches.sh so we always use the same sort order, I'll
fix that now.

 >> Talking of which, why is this patch not in the fixes directory? Does
 >> Denys not consider it important enough?

 Raúl>   That's probably something to ask him. I found this bug by
 Raúl> chance and I don't consider it critical in any way, altough I
 Raúl> haven't explored further implications. This can be reproduced
 Raúl> when you invoke mdev without parameters. It will SIGSEGV since,
 Raúl> as per current 1.21.0 code, you'll check for ACTION environment
 Raúl> variable, which doesn't exist. This means a null pointer which is
 Raúl> passed to index_in_strings which, in turn, passes it to the
 Raúl> uclibc strcmp function that is unable to handle this case
 Raúl> gracefully.

Could you bring it up on the busybox list and suggest it gets added to 1.21-fixes, please?

Patch

diff --git a/package/busybox/1.21.0/busybox-1.21.0-mdev.patch b/package/busybox/1.21.0/busybox-1.21.0-mdev_1.patch
similarity index 100%
rename from package/busybox/1.21.0/busybox-1.21.0-mdev.patch
rename to package/busybox/1.21.0/busybox-1.21.0-mdev_1.patch
diff --git a/package/busybox/1.21.0/busybox-1.21.0-mdev_2_check_ACTION.patch b/package/busybox/1.21.0/busybox-1.21.0-mdev_2_check_ACTION.patch
new file mode 100644
index 0000000..f731cc0
--- /dev/null
+++ b/package/busybox/1.21.0/busybox-1.21.0-mdev_2_check_ACTION.patch
@@ -0,0 +1,32 @@ 
+From d35cbad0efaa57bf7c5280e62825966f7757906a Mon Sep 17 00:00:00 2001
+From: Denys Vlasenko <vda.linux@googlemail.com>
+Date: Tue, 02 Apr 2013 12:37:06 +0000
+Subject: mdev: call index_in_strings on $ACTION only after we checked it for NULL
+
+Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
+---
+diff --git a/util-linux/mdev.c b/util-linux/mdev.c
+index 5fe6bbb..1d74136 100644
+--- a/util-linux/mdev.c
++++ b/util-linux/mdev.c
+@@ -1060,15 +1060,15 @@ int mdev_main(int argc UNUSED_PARAM, char **argv)
+ 		 * ACTION can be "add", "remove", "change"
+ 		 * DEVPATH is like "/block/sda" or "/class/input/mice"
+ 		 */
+-		action = getenv("ACTION");
+-		op = index_in_strings(keywords, action);
+ 		env_devname = getenv("DEVNAME"); /* can be NULL */
+-		env_devpath = getenv("DEVPATH");
+ 		G.subsystem = getenv("SUBSYSTEM");
++		action = getenv("ACTION");
++		env_devpath = getenv("DEVPATH");
+ 		if (!action || !env_devpath /*|| !G.subsystem*/)
+ 			bb_show_usage();
+ 		fw = getenv("FIRMWARE");
+ 		seq = getenv("SEQNUM");
++		op = index_in_strings(keywords, action);
+ 
+ 		my_pid = getpid();
+ 		open_mdev_log(seq, my_pid);
+--
+cgit v0.9.1