Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 17 Apr 2013 01:08:01 +0200
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: Segfaults probably caused by DEBUG code in memory.c

On 17 Apr, 2013, at 0:23 , Frank Dittrich <frank_dittrich@...mail.com> wrote:
> On 04/17/2013 12:14 AM, magnum wrote:
>> On 16 Apr, 2013, at 23:37 , Frank Dittrich <frank_dittrich@...mail.com> wrote:
>>> On 04/16/2013 09:56 PM, jfoug wrote:
>>>> In hindsight, in debug mode, you might have gotten the same results by doing
>>>> this:
>>>> 
>>>> #if DEBUG
>>>> #undef MEM_ALLOC_SIZE
>>>> #define MEM_ALLOC_SIZE 0
>>>> #endif
>>> 
>>> I really like this solution, because it leaves mem_alloc_tiny (which is
>>> complex enough as it is) unchanged.
>>> 
>>> May be even better is to do this in memory.h directly:
>>> 
>>> #if DEBUG
>>> /* turn mem_alloc_tiny() into a normal alloc,
>>> * to better track problems.
>>> */
>>> #define MEM_ALLOC_SIZE			0
>>> #else
>>> #define MEM_ALLOC_SIZE			0x10000
>> 
>> I bought your arguments and did this, even commited it without testing (silly me). But it segfaults for me in unstable, and hangs in bleeding, both in dynamic_1.
>> 
>> I reverted it. You can try for yourself, unstable is 3b53da3 and bleeding is 83a9e10.
> 
> My fault:
> 
> dynamic_fmt.c:1800:             pSaltDataBuf = pNextSaltDataBuf =
> mem_alloc_tiny(MEM_ALLOC_SIZE*6, MEM_ALIGN_NON
> dynamic_fmt.c:1801:             nSaltDataBuf = MEM_ALLOC_SIZE*6;
> 
> I should have checked the usage of MEM_ALLOC_SIZE before suggesting the
> change.

Aha... I thought it was some worse issue within mem_alloc_tiny and the remainder of last alloc. In bleeding (they do differ) it really is. Here's the bleeding version (actually the bleeding core version):

31      void *mem_alloc_tiny(size_t size, size_t align)
32      {
33              static char *buffer = NULL;
34              static size_t bufree = 0;
35              size_t mask;
36              char *p;
37      
38      #if ARCH_ALLOWS_UNALIGNED
39              if (mem_saving_level > 2)
40                      align = MEM_ALIGN_NONE;
41      #endif
42      
43              mask = align - 1;
44      
45              do {
46                      if (buffer) {
47                              size_t need =
48                                  size + mask - (((size_t)buffer + mask) & mask);
49                              if (bufree >= need) {
50                                      p = buffer;
51                                      p += mask;
52                                      p -= (size_t)p & mask;
53                                      bufree -= need;
54                                      buffer = p + size;
55                                      return p;
56                              }
57                      }
58      
59                      if (size + mask > MEM_ALLOC_SIZE ||
60                          bufree > MEM_ALLOC_MAX_WASTE)
61                              break;
62      
63                      buffer = mem_alloc(MEM_ALLOC_SIZE);
64                      bufree = MEM_ALLOC_SIZE;
65              } while (1);
66      
67              p = mem_alloc(size + mask);
68              p += mask;
69              p -= (size_t)p & mask;
70              return p;
71      }

Now, if MEM_ALLOC_SIZE is 0 we will allocate 0 bytes in line 63, and never get out of that loop.

In unstable this does not happen but there is another problem (even if not doing the #ifdef in memory.h): If we allocate something with a large alignment like 64 but malloc() happens to return an already aligned buffer, we'll end up having bufree==64. At next call and providing we ask for <= 64 bytes we will get memory from that. This contradicts the point with my -DDEBUG behaviour.

I suggest we keep the current -DDEBUG code as-is for now.

magnum

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.