Re: [PATCH v2] locale: don't crash if locale-archive contains all zeros

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/03/2013 08:23 AM, Aurelien Jarno wrote:
> On Tue, Dec 03, 2013 at 12:40:54PM +0100, OndÅej BÃlka wrote:
>> On Tue, Dec 03, 2013 at 12:21:33PM +0100, Andreas Schwab wrote:
>>> Aurelien Jarno <aurelien@aurel32.net> writes:
>>>
>>>> diff --git a/locale/loadarchive.c b/locale/loadarchive.c
>>>> index 70136dc..f723780 100644
>>>> --- a/locale/loadarchive.c
>>>> +++ b/locale/loadarchive.c
>>>> @@ -274,6 +274,10 @@ _nl_load_locale_from_archive (int category, const char **namep)
>>>>    namehashtab = (struct namehashent *) ((char *) head
>>>>  					+ head->namehash_offset);
>>>>  
>>>> +  /* Avoid division by 0 if the file is corrupted.  */
>>>> +  if (__glibc_unlikely (head->namehash_size == 0))
>>>> +    goto close_and_out;
>>>> +
>>>>    idx = hval % head->namehash_size;
>>>>    incr = 1 + hval % (head->namehash_size - 2);
>>>
>>> That won't help for head->namehash_size == 2, or any other corruptions.
> 
> Indeed it will still crash for head->namehash_size == 2, it's something
> I missed. For other corruptions, they are handled later in the code.

I don't think you didn't miss anything.

The point of the patch as I understood it was to detect the situation
where you had a file full of zeroes. 

That corruption is easy to explain since the file might have just been
created, and might be full of zeroes, and then the server might crash.

>> Which is less common zeroed file. Proper solution would be starting
>> files with magic constant which is too late to add.
> 
> Isn't it possible to break the format between releases, iow people are
> not supposed to rebuild the locales when installing a new libc?

In Fedora we always rebuild the archive.

I believe that you are correct but we have no official consensus on
that topic. The next step here would be:

* Email libc-alpha, and CC distributions from wiki MAINTAINERS
* Ask for consensus on the fact that the archive needs rebuilding
  after each upgrade to support new formats.
* Post a patch that adds a magic header after getting consensus.

It still doesn't mean your code is invalid, only that it catches
one type of problem.

>> Do you have idea to detect corruption other than changing check to
>>
>> head->namehash_size == 0 || head->namehash_size == 2
> 
> I don't think 1 is a value that can be used in practice, so what about 
> head->namehash_size <= 2 ?

I agree, but you'd have to double check and show proof that it can't
happen (is underflow allowed here?).

Cheers,
Carlos.