Re: ELF bugfixes

[Robert Millan <rmh@aybabtu.com>]

On Sat, Mar 14, 2009 at 12:01:13AM +0100, phcoder wrote:
> Robert Millan wrote:
>> On Fri, Mar 13, 2009 at 09:41:42PM +0100, phcoder wrote:
>>> Robert Millan wrote:
>>>> On Wed, Mar 11, 2009 at 10:21:41PM +0100, phcoder wrote:
>>>>> Robert Millan wrote:
>>>>>> On Mon, Mar 02, 2009 at 01:35:06AM +0100, phcoder wrote:
>>>>>>> +	* include/grub/elf.h: added missing attributes
>>>>>> This should be a bit more descriptive.
>>>>>>
>>>>>>>    for (i = 0; i < ehdr->e_phnum; i++)
>>>>>>>      if (phdr(i)->p_type == PT_LOAD && phdr(i)->p_filesz != 0)
>>>>>>>        {
>>>>>>> -	if (phdr(i)->p_paddr < phdr(lowest_segment)->p_paddr)
>>>>>>> +	if (lowest_segment == -1 +	    || phdr(i)->p_paddr <   
>>>>>>> phdr(lowest_segment)->p_paddr)
>>>>>>>  	  lowest_segment = i;
>>>>>>> -	if (phdr(i)->p_paddr > phdr(highest_segment)->p_paddr)
>>>>>>> +	if (highest_segment == -1
>>>>>>> +	    || phdr(i)->p_paddr > phdr(highest_segment)->p_paddr)
>>>>>>>  	  highest_segment = i;
>>>>>>>        }
>>>>>> Why?
>>>>> Because if first segment doesn't have the PT_LOAD attribute set 
>>>>> then it  should be considered in this comparison
>>>> But you didn't remove the PT_LOAD check.  And in the routine below that
>>>> does the actual segment load, we still check for PT_LOAD.  Those should be
>>>> consistent, right?
>>>>
>>> No I expressed myself badly. Original code assumed that first segment 
>>>  has PT_LOAD always set (lowest_segment is 0 initally). I removed 
>>> this  assumption
>>
>> Why do we care about non-PT_LOAD segments?
>
> We don't but without this fix non-PT_LOAD segment 1 wasn't correctly ignored

Oh, of course...  This part of the patch is fine.  Perhaps a comment would be
a good idea, so we don't forget.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."