[Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Robert Reif]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: ptimer.c.diff.txt --]
[-- Type: text/plain, Size: 383 bytes --]

diff -p -u -r1.5 ptimer.c
--- hw/ptimer.c	17 Nov 2007 17:14:47 -0000	1.5
+++ hw/ptimer.c	3 Jan 2008 02:27:18 -0000
@@ -185,6 +185,8 @@ ptimer_state *ptimer_init(QEMUBH *bh)
     ptimer_state *s;
 
     s = (ptimer_state *)qemu_mallocz(sizeof(ptimer_state));
+    if (!s)
+        return NULL;
     s->bh = bh;
     s->timer = qemu_new_timer(vm_clock, ptimer_tick, s);
     return s;

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Paul Brook]

>      s = (ptimer_state *)qemu_mallocz(sizeof(ptimer_state));
> +    if (!s)
> +        return NULL;

None of the callers bother to check the return value, And even if they did I 
don't think there's any point trying to gracefully handle OOM.  Just abort 
and be done with it.

I suggest guaranteeing that qemu_malloc will never return NULL, and removing 
the null checks from all the various users.

Paul

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Robert Reif]

Paul Brook wrote:

>>     s = (ptimer_state *)qemu_mallocz(sizeof(ptimer_state));
>>+    if (!s)
>>+        return NULL;
>>    
>>
>
>None of the callers bother to check the return value, And even if they did I 
>don't think there's any point trying to gracefully handle OOM.  Just abort 
>and be done with it.
>  
>
I am in the process of fixing the sparc ptimer caller to gracefully 
handle OOM.
We currently don't check the return value in the init function where the new
timer is created but do check it wherever it is used which is backwards and
wasteful.

You would prefer that qemu just segfaults rather than die gracefully?

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Paul Brook]

> We currently don't check the return value in the init function where the
> new timer is created but do check it wherever it is used which is backwards
> and wasteful.
>
> You would prefer that qemu just segfaults rather than die gracefully?

I think qemu should die before it returns from qemu_malloc.

Having to check every return value is extremely tedious and (as you've proved) 
easy to miss.  If the allocation fails we don't have any viable alternatives, 
so we may as well stop right there.

Paul

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Markus Hitter]

Am 03.01.2008 um 15:02 schrieb Paul Brook:

> Having to check every return value is extremely tedious and (as  
> you've proved)
> easy to miss.

Checking every return value is a measure for programming reliable code.

> If the allocation fails we don't have any viable alternatives, so  
> we may as well stop right there.

Stop != segfault? What about a meaningful exit message? What if the  
code doesn't segfault but runs havok?


Markus

- - - - - - - - - - - - - - - - - - -
Dipl. Ing. Markus Hitter
http://www.jump-ing.de/

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Paul Brook]

On Friday 04 January 2008, Markus Hitter wrote:
> Am 03.01.2008 um 15:02 schrieb Paul Brook:
> > Having to check every return value is extremely tedious and (as
> > you've proved) easy to miss.
>
> Checking every return value is a measure for programming reliable code.

Never failing is even more reliable.

> > If the allocation fails we don't have any viable alternatives, so
> > we may as well stop right there.
>
> Stop != segfault? 

Yes.

> What about a meaningful exit message? 

"Out of memory" is a fairly comprehensive description of the problem.
In fact I'd say it's much more informative than "<random widget the user 
doesn't know or care about> failed to initialize".

Paul

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Robert Reif]

Paul Brook wrote:

>>What about a meaningful exit message? 
>>    
>>
>
>"Out of memory" is a fairly comprehensive description of the problem.
>In fact I'd say it's much more informative than "<random widget the user 
>doesn't know or care about> failed to initialize".
>  
>
If the user requested a target parameter that is beyond the capabilities
of the host system, a meaningful error message can be generated that
instructs the user on how to possibly correct the problem.

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Rob Landley]

On Friday 04 January 2008 11:29:27 Markus Hitter wrote:
> Am 03.01.2008 um 15:02 schrieb Paul Brook:
> > Having to check every return value is extremely tedious and (as
> > you've proved)
> > easy to miss.
>
> Checking every return value is a measure for programming reliable code.

There's an old tounge-in-cheek programming rule: "Never test for an error 
condition you don't know how to handle."  An example of "ha ha, only 
serious".

Not only is defensive programming a huge source of bloat, but I've had to deal 
with a lot of programs where what broke _was_ some defensive check, and the 
code would have worked just fine if they just hadn't been so paranoid.  (So 
the return value of close() is nonzero.  What do you _do_ about it?  Calling 
abort() is not necessarily the sane response.)

I've got a shell script where bash spits out an error from an
  EVVVAR="$(thing | thing2)"
construct, despite getting the right answer.  Why?  Because thing2 exits after 
consuming all its data, and the left half of the pipeline outlives it by a 
milisecond or so and then errors out when it tries to flush and close stdout 
(which has already delivered all its data, but it gets an error return from 
doing this anyway and darn it, it's going to tell you about it!)

I think that to shut it up I need to rephrase it as:
  ENVVAR="$(thing 2>/dev/null | thing2)"
but to be honest, I haven't bothered.

> > If the allocation fails we don't have any viable alternatives, so
> > we may as well stop right there.
>
> Stop != segfault? What about a meaningful exit message? What if the
> code doesn't segfault but runs havok?

On modern operating systems, allocations only return zero when you exhaust 
virtual memory.  Returning nonzero doesn't mean you have enough memory, 
because it's given you a redundant copy on write mapping of the zero page and 
will fault in physical pages when you write to 'em, which has _no_ return 
value.  Instead, the out of memory killer will shoot your program in the head 
in the middle of it's run, and your only indication of a real out of memory 
status is "kill -9".

So on a modern system, defensive programming is also somewhat futile.

Just FYI.

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Paul Brook]

> On modern operating systems, allocations only return zero when you exhaust
> virtual memory.  Returning nonzero doesn't mean you have enough memory,
> because it's given you a redundant copy on write mapping of the zero page
> and will fault in physical pages when you write to 'em, which has _no_
> return value.  Instead, the out of memory killer will shoot your program in
> the head in the middle of it's run

Decent operating systems allow the system administrator gets to choose how 
optimistic memory allocation is. You're describing wildly-optimistic mode, 
which is often but not always the default.

Paul

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Rob Landley]

On Friday 04 January 2008 19:07:58 Paul Brook wrote:
> > On modern operating systems, allocations only return zero when you
> > exhaust virtual memory.  Returning nonzero doesn't mean you have enough
> > memory, because it's given you a redundant copy on write mapping of the
> > zero page and will fault in physical pages when you write to 'em, which
> > has _no_ return value.  Instead, the out of memory killer will shoot your
> > program in the head in the middle of it's run
>
> Decent operating systems allow the system administrator gets to choose how
> optimistic memory allocation is. You're describing wildly-optimistic mode,
> which is often but not always the default.

True, but if you completely disable overcommit then fork() a large process and 
exec() a small one, you haven't got enough memory even though you're not 
really _using_ any to do so.

You can disable overcommit and give the system an egregious amount of swap 
space, but then your pathological case is the system going into swap 
thrashing la-la land and essentially freezing (advancing at 0.1% of its 
normal rate, if that, for _hours_) instead of killing some runaway processes 
(or rebooting) and recovering.  Not necessarily and improvement, especially 
if you're the one with the pager.

It is alas, not a simple problem to get right.  fork() and exec() being 
separate system calls isn't always an improvement over a combined one.  
(Espeically since exec() needs a file, not a file handle.  You can't re-exec 
your current process unless you can find and reopen it, you can't exec() from 
a pipe...  And then there's nommu vfork(), always fun...)

> Paul

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Markus Hitter]

Am 05.01.2008 um 03:47 schrieb Rob Landley:

> You can disable overcommit and give the system an egregious amount  
> of swap
> space, but then your pathological case is the system going into swap
> thrashing la-la land and essentially freezing (advancing at 0.1% of  
> its
> normal rate, if that, for _hours_) instead of killing some runaway  
> processes
> (or rebooting) and recovering.

Quite obviously, we have very different expectations on what  
executables should do. I expect code to be as reliable as possible,  
to be careful when aquiring resources and if a process would ever  
happen to make my computer spontanuously reboot, I'd throw that  
binary as far away as possible and warn everybody else not to even  
touch this thing.

One of the major reasons to run emulators is to protect against such  
sloppy code, btw.


Markus

- - - - - - - - - - - - - - - - - - -
Dipl. Ing. Markus Hitter
http://www.jump-ing.de/

Re: [Qemu-devel] [PATCH] fix possible NULL pointer use in hw/ptimer.c

[Avi Kivity]

Markus Hitter wrote:
>
> Quite obviously, we have very different expectations on what 
> executables should do. I expect code to be as reliable as possible, to 
> be careful when aquiring resources and if a process would ever happen 
> to make my computer spontanuously reboot, I'd throw that binary as far 
> away as possible and warn everybody else not to even touch this thing.
>

If a user program causes your computer to spontaneously reboot, you 
should throw away your OS instead of that program.

-- 
error compiling committee.c: too many arguments to function