grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Bug fix for loadenv.c


From: Seth Goldberg
Subject: Re: [PATCH] Bug fix for loadenv.c
Date: Thu, 03 Sep 2009 12:45:32 -0700 (PDT)
User-agent: Alpine 2.00 (GSO 1167 2008-08-23)

Hi,

Maybe that compiler has been configured with -fdelete-null-pointer-checks as the default?

 --S

Quoting Bean, who wrote the following on Fri, 4 Sep 2009:

Hi,

This is the bug fix for command/loadenv.c. Here is a small program to
illustrate what happen:

#include <stdio.h>
#include <stdlib.h>

struct data
{
 struct data *next;
 int length;
};

struct data *
new_data ()
{
 struct data *p;

 p = malloc (sizeof (*p));
 p->length = 1;
 p->next = 0;

 return p;
}

int
main ()
{
 int index;
 struct data *head, *p;

 head = new_data ();
 for (p = head, index = 0; p; p = p->next, index += p->length)
   printf ("%d\n", index);

 return 0;
}

Compile it with gcc-4.0 from OSX:
i686-apple-dawin8-gcc-4.0.1 -Os test.c

The assembly dump shows:

L4:
        movl    %edi, 4(%esp)
        movl    -28(%ebp), %eax
        movl    %eax, (%esp)
        call    L_printf$stub
        movl    (%esi), %esi
        addl    4(%esi), %edi
        jmp     L4

Why would it uses unconditional jumps ? I think it's some over
optimization in gcc. We have index += p->length after p = p->next, if
p is null, p->length would be invalid, so gcc figure that p would
never be null and the end condition is always false. In fact, the bug
is in the code itself, even if it tests p, in OS environment, access
null pointer is not allowed, so index += p->length can't be executed
anyway.

The correct way is:

 for (p = head, index = 0; p;)
 {
   printf ("%d\n", index);
  p = p->next;
 if (p)
  index += p->length;
 }


--
Bean

gitgrub home: http://github.com/grub/grub/
my fork page: http://github.com/bean123/grub/





reply via email to

[Prev in Thread] Current Thread [Next in Thread]