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

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

[Raúl Sánchez Siles]

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

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
-- 
1.7.10.4

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

[Peter Korsgaard]

>>>>> "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?

-- 
Bye, Peter Korsgaard

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

[Raúl Sánchez Siles]

  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,

-- 
     Ra?l S?nchez Siles
----->Proud Debian user<-----
Linux registered user #416098
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130612/c15bfe39/attachment-0001.asc>

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

[Peter Korsgaard]

>>>>> "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?

-- 
Bye, Peter Korsgaard