* [PATCH] fix buffer overflow in util_run_program()
@ 2009-09-04 19:54 Florian Zumbiehl
2009-09-05 3:29 ` Andrey Borzenkov
` (11 more replies)
0 siblings, 12 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-04 19:54 UTC (permalink / raw)
To: linux-hotplug
Hi,
...
Reading your high-quality code starts getting a bit boring, so I guess that
I won't waste any more of your valuable time for now, except for finishing
the threads I started.
Florian
diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index 64203a8..c309945 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const char *command, char **envp,
pid_t pid;
char arg[UTIL_PATH_SIZE];
char program[UTIL_PATH_SIZE];
- char *argv[(sizeof(arg) / 2) + 1];
+ char *argv[sizeof(arg) + 1];
int devnull;
int i;
int err = 0;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
@ 2009-09-05 3:29 ` Andrey Borzenkov
2009-09-05 4:25 ` Florian Zumbiehl
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrey Borzenkov @ 2009-09-05 3:29 UTC (permalink / raw)
To: linux-hotplug
[-- Attachment #1: Type: text/plain, Size: 784 bytes --]
On Friday 04 of September 2009 23:54:14 Florian Zumbiehl wrote:
> Hi,
>
> ...
>
> Reading your high-quality code starts getting a bit boring, so I
> guess that I won't waste any more of your valuable time for now,
> except for finishing the threads I started.
>
> Florian
>
> diff --git a/libudev/libudev-util-private.c
> b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> --- a/libudev/libudev-util-private.c
> +++ b/libudev/libudev-util-private.c
> @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> char *command, char **envp, pid_t pid;
> char arg[UTIL_PATH_SIZE];
> char program[UTIL_PATH_SIZE];
> - char *argv[(sizeof(arg) / 2) + 1];
> + char *argv[sizeof(arg) + 1];
Could you give example when this overflows?
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
2009-09-05 3:29 ` Andrey Borzenkov
@ 2009-09-05 4:25 ` Florian Zumbiehl
2009-09-05 4:34 ` Andrey Borzenkov
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-05 4:25 UTC (permalink / raw)
To: linux-hotplug
Hi,
> > diff --git a/libudev/libudev-util-private.c
> > b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > --- a/libudev/libudev-util-private.c
> > +++ b/libudev/libudev-util-private.c
> > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> > char *command, char **envp, pid_t pid;
> > char arg[UTIL_PATH_SIZE];
> > char program[UTIL_PATH_SIZE];
> > - char *argv[(sizeof(arg) / 2) + 1];
> > + char *argv[sizeof(arg) + 1];
>
> Could you give example when this overflows?
UTIL_PATH_SIZE-1 spaces.
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
2009-09-05 3:29 ` Andrey Borzenkov
2009-09-05 4:25 ` Florian Zumbiehl
@ 2009-09-05 4:34 ` Andrey Borzenkov
2009-09-05 4:49 ` Florian Zumbiehl
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrey Borzenkov @ 2009-09-05 4:34 UTC (permalink / raw)
To: linux-hotplug
[-- Attachment #1: Type: Text/Plain, Size: 713 bytes --]
On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
> Hi,
>
> > > diff --git a/libudev/libudev-util-private.c
> > > b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > > --- a/libudev/libudev-util-private.c
> > > +++ b/libudev/libudev-util-private.c
> > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> > > char *command, char **envp, pid_t pid;
> > > char arg[UTIL_PATH_SIZE];
> > > char program[UTIL_PATH_SIZE];
> > > - char *argv[(sizeof(arg) / 2) + 1];
> > > + char *argv[sizeof(arg) + 1];
> >
> > Could you give example when this overflows?
>
> UTIL_PATH_SIZE-1 spaces.
>
Please try to understand what code you are fixing does.
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (2 preceding siblings ...)
2009-09-05 4:34 ` Andrey Borzenkov
@ 2009-09-05 4:49 ` Florian Zumbiehl
2009-09-05 10:50 ` Alan Jenkins
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-05 4:49 UTC (permalink / raw)
To: linux-hotplug
Hi,
> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
> > Hi,
> >
> > > > diff --git a/libudev/libudev-util-private.c
> > > > b/libudev/libudev-util-private.c index 64203a8..c309945 100644
> > > > --- a/libudev/libudev-util-private.c
> > > > +++ b/libudev/libudev-util-private.c
> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
> > > > char *command, char **envp, pid_t pid;
> > > > char arg[UTIL_PATH_SIZE];
> > > > char program[UTIL_PATH_SIZE];
> > > > - char *argv[(sizeof(arg) / 2) + 1];
> > > > + char *argv[sizeof(arg) + 1];
> > >
> > > Could you give example when this overflows?
> >
> > UTIL_PATH_SIZE-1 spaces.
> >
>
> Please try to understand what code you are fixing does.
No, of course not.
Now, what am I missing? I obviously do not understand much of how udev
works, but if the code of this function is not somewhat pointless, then
how would there not be a potential buffer overflow?
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (3 preceding siblings ...)
2009-09-05 4:49 ` Florian Zumbiehl
@ 2009-09-05 10:50 ` Alan Jenkins
2009-09-05 17:21 ` Florian Zumbiehl
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Alan Jenkins @ 2009-09-05 10:50 UTC (permalink / raw)
To: linux-hotplug
On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
>> > Hi,
>> >
>> > > > diff --git a/libudev/libudev-util-private.c
>> > > > b/libudev/libudev-util-private.c index 64203a8..c309945 100644
>> > > > --- a/libudev/libudev-util-private.c
>> > > > +++ b/libudev/libudev-util-private.c
>> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
>> > > > char *command, char **envp, pid_t pid;
>> > > > char arg[UTIL_PATH_SIZE];
>> > > > char program[UTIL_PATH_SIZE];
>> > > > - char *argv[(sizeof(arg) / 2) + 1];
>> > > > + char *argv[sizeof(arg) + 1];
>> > >
>> > > Could you give example when this overflows?
>> >
>> > UTIL_PATH_SIZE-1 spaces.
>> >
>>
>> Please try to understand what code you are fixing does.
>
> No, of course not.
>
> Now, what am I missing? I obviously do not understand much of how udev
> works, but if the code of this function is not somewhat pointless, then
> how would there not be a potential buffer overflow?
>
> Florian
Running "ls -l" (two spaces) should be equivalent to "ls -l" (one
space). arg filled with spaces should be more or less equivalent to
arg = "". If it's not - then that's the real bug.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (4 preceding siblings ...)
2009-09-05 10:50 ` Alan Jenkins
@ 2009-09-05 17:21 ` Florian Zumbiehl
2009-09-05 18:41 ` Alan Jenkins
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-05 17:21 UTC (permalink / raw)
To: linux-hotplug
Hi,
> > Now, what am I missing? I obviously do not understand much of how udev
> > works, but if the code of this function is not somewhat pointless, then
> > how would there not be a potential buffer overflow?
> >
> > Florian
>
> Running "ls -l" (two spaces) should be equivalent to "ls -l" (one
> space). arg filled with spaces should be more or less equivalent to
> arg = "". If it's not - then that's the real bug.
well, I don't want to get into fixing semantic bugs, as there generally
doesn't seem to be much of a hint as to what the intended semantics are -
except that you wonder how the code's semantics could actually be
intentional. So I would suggest fixing the buffer overflow for now, until
someone feels like taking care of the semantic bug.
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (5 preceding siblings ...)
2009-09-05 17:21 ` Florian Zumbiehl
@ 2009-09-05 18:41 ` Alan Jenkins
2009-09-05 19:07 ` Andrey Borzenkov
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Alan Jenkins @ 2009-09-05 18:41 UTC (permalink / raw)
To: linux-hotplug
On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> Hi,
>
>> > Now, what am I missing? I obviously do not understand much of how udev
>> > works, but if the code of this function is not somewhat pointless, then
>> > how would there not be a potential buffer overflow?
>> >
>> > Florian
>>
>> Running "ls -l" (two spaces) should be equivalent to "ls -l" (one
>> space). arg filled with spaces should be more or less equivalent to
>> arg = "". If it's not - then that's the real bug.
>
> well, I don't want to get into fixing semantic bugs, as there generally
> doesn't seem to be much of a hint as to what the intended semantics are -
> except that you wonder how the code's semantics could actually be
> intentional. So I would suggest fixing the buffer overflow for now, until
> someone feels like taking care of the semantic bug.
My point was that I don't see any such semantic bug; I can't see where
the overflow would come from.
As far as I can see, the code uses strsep() which will correctly
interpret a string of spaces as containing no tokens - and return
NULL.
If I'm right, there's a different semantic bug - the use of strsep()
to find a closing quote, which will fail for strings like
' a '' b '
Regards
Alan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (6 preceding siblings ...)
2009-09-05 18:41 ` Alan Jenkins
@ 2009-09-05 19:07 ` Andrey Borzenkov
2009-09-05 20:39 ` Florian Zumbiehl
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Andrey Borzenkov @ 2009-09-05 19:07 UTC (permalink / raw)
To: linux-hotplug
[-- Attachment #1: Type: Text/Plain, Size: 1532 bytes --]
On Saturday 05 of September 2009 22:41:27 Alan Jenkins wrote:
> On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
> > Hi,
> >
> >> > Now, what am I missing? I obviously do not understand much of
> >> > how udev works, but if the code of this function is not somewhat
> >> > pointless, then how would there not be a potential buffer
> >> > overflow?
> >> >
> >> > Florian
> >>
> >> Running "ls -l" (two spaces) should be equivalent to "ls -l" (one
> >> space). arg filled with spaces should be more or less equivalent
> >> to arg = "". If it's not - then that's the real bug.
> >
> > well, I don't want to get into fixing semantic bugs, as there
> > generally doesn't seem to be much of a hint as to what the intended
> > semantics are - except that you wonder how the code's semantics
> > could actually be intentional. So I would suggest fixing the buffer
> > overflow for now, until someone feels like taking care of the
> > semantic bug.
>
> My point was that I don't see any such semantic bug; I can't see
> where the overflow would come from.
>
> As far as I can see, the code uses strsep() which will correctly
> interpret a string of spaces as containing no tokens - and return
> NULL.
>
> If I'm right, there's a different semantic bug - the use of strsep()
> to find a closing quote, which will fail for strings like
>
> ' a '' b '
>
If this is assumed to be two arguments ' a ' and ' b ', this function
works correctly. What is really not possible, is to quote the quote.
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (7 preceding siblings ...)
2009-09-05 19:07 ` Andrey Borzenkov
@ 2009-09-05 20:39 ` Florian Zumbiehl
2009-09-06 16:34 ` Kay Sievers
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-05 20:39 UTC (permalink / raw)
To: linux-hotplug
Hi,
> > well, I don't want to get into fixing semantic bugs, as there generally
> > doesn't seem to be much of a hint as to what the intended semantics are -
> > except that you wonder how the code's semantics could actually be
> > intentional. So I would suggest fixing the buffer overflow for now, until
> > someone feels like taking care of the semantic bug.
>
> My point was that I don't see any such semantic bug; I can't see where
> the overflow would come from.
>
> As far as I can see, the code uses strsep() which will correctly
> interpret a string of spaces as containing no tokens - and return
> NULL.
>
> If I'm right, there's a different semantic bug - the use of strsep()
> to find a closing quote, which will fail for strings like
>
> ' a '' b '
| $ cat foo.c
|
| #include <string.h>
| #include <stdio.h>
|
| int main(){
| char s[]=" ",*p=s;
| while(p)printf("|%s|\n",strsep(&p," "));
| return 0;
| }
|
| $ gcc -o foo foo.c
| $ ./foo
| ||
| ||
| ||
| ||
| $
actually, I don't really know for sure what the intended semantics of
strsep() are, but assuming that the glibc implementation is not
majorly broken, I would argue that there is a buffer overflow in
that code ;-)
Florian
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program()
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (8 preceding siblings ...)
2009-09-05 20:39 ` Florian Zumbiehl
@ 2009-09-06 16:34 ` Kay Sievers
2009-09-07 18:18 ` [PATCH] fix buffer overflow in util_run_program(), #2 Florian Zumbiehl
2009-09-08 19:43 ` Kay Sievers
11 siblings, 0 replies; 13+ messages in thread
From: Kay Sievers @ 2009-09-06 16:34 UTC (permalink / raw)
To: linux-hotplug
On Sat, Sep 5, 2009 at 12:50, Alan Jenkins
<sourcejedi.lkml@googlemail.com> wrote:
> On 9/5/09, Florian Zumbiehl <florz@florz.de> wrote:
>>> On Saturday 05 of September 2009 08:25:01 Florian Zumbiehl wrote:
>>> > > > diff --git a/libudev/libudev-util-private.c
>>> > > > b/libudev/libudev-util-private.c index 64203a8..c309945 100644
>>> > > > --- a/libudev/libudev-util-private.c
>>> > > > +++ b/libudev/libudev-util-private.c
>>> > > > @@ -268,7 +268,7 @@ int util_run_program(struct udev *udev, const
>>> > > > char *command, char **envp, pid_t pid;
>>> > > > char arg[UTIL_PATH_SIZE];
>>> > > > char program[UTIL_PATH_SIZE];
>>> > > > - char *argv[(sizeof(arg) / 2) + 1];
>>> > > > + char *argv[sizeof(arg) + 1];
>>> > >
>>> > > Could you give example when this overflows?
>>> >
>>> > UTIL_PATH_SIZE-1 spaces.
>>> >
>>>
>>> Please try to understand what code you are fixing does.
>>
>> No, of course not.
>>
>> Now, what am I missing? I obviously do not understand much of how udev
>> works, but if the code of this function is not somewhat pointless, then
>> how would there not be a potential buffer overflow?
>
> Running "ls -l" (two spaces) should be equivalent to "ls -l" (one
> space). arg filled with spaces should be more or less equivalent to
> arg = "". If it's not - then that's the real bug.
Changed it to skip multiple consecutive spaces.
Thanks,
Kay
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] fix buffer overflow in util_run_program(), #2
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (9 preceding siblings ...)
2009-09-06 16:34 ` Kay Sievers
@ 2009-09-07 18:18 ` Florian Zumbiehl
2009-09-08 19:43 ` Kay Sievers
11 siblings, 0 replies; 13+ messages in thread
From: Florian Zumbiehl @ 2009-09-07 18:18 UTC (permalink / raw)
To: linux-hotplug
Hi,
I'm not sure how likely it is for UTIL_PATH_SIZE to have an odd value
(maybe it has right now? :-), but I guess making this universally correct
doesn't hurt ...
Florian
diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index fb64c13..e0670db 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -251,7 +251,7 @@ int util_run_program(struct udev *udev, const char *command, char **envp,
pid_t pid;
char arg[UTIL_PATH_SIZE];
char program[UTIL_PATH_SIZE];
- char *argv[(sizeof(arg) / 2) + 1];
+ char *argv[((sizeof(arg) + 1) / 2) + 1];
int devnull;
int i;
int err = 0;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] fix buffer overflow in util_run_program(), #2
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
` (10 preceding siblings ...)
2009-09-07 18:18 ` [PATCH] fix buffer overflow in util_run_program(), #2 Florian Zumbiehl
@ 2009-09-08 19:43 ` Kay Sievers
11 siblings, 0 replies; 13+ messages in thread
From: Kay Sievers @ 2009-09-08 19:43 UTC (permalink / raw)
To: linux-hotplug
On Mon, Sep 7, 2009 at 20:18, Florian Zumbiehl<florz@florz.de> wrote:
> I'm not sure how likely it is for UTIL_PATH_SIZE to have an odd value
> (maybe it has right now? :-), but I guess making this universally correct
> doesn't hurt ...
Applied.
Thanks,
Kay
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2009-09-08 19:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-04 19:54 [PATCH] fix buffer overflow in util_run_program() Florian Zumbiehl
2009-09-05 3:29 ` Andrey Borzenkov
2009-09-05 4:25 ` Florian Zumbiehl
2009-09-05 4:34 ` Andrey Borzenkov
2009-09-05 4:49 ` Florian Zumbiehl
2009-09-05 10:50 ` Alan Jenkins
2009-09-05 17:21 ` Florian Zumbiehl
2009-09-05 18:41 ` Alan Jenkins
2009-09-05 19:07 ` Andrey Borzenkov
2009-09-05 20:39 ` Florian Zumbiehl
2009-09-06 16:34 ` Kay Sievers
2009-09-07 18:18 ` [PATCH] fix buffer overflow in util_run_program(), #2 Florian Zumbiehl
2009-09-08 19:43 ` Kay Sievers
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.