573688 - sqlite leaks a bunch

Status: RESOLVED INVALID

(Toolkit :: Storage, defect)

Description

[Jesse Ruderman]

Attachment: valgrind output

Just starting up and shutting down, I get lots of leaks from sqlite :(

Using:
http://hg.mozilla.org/mozilla-central/rev/0c940e50d738
ac_add_options --enable-debug
ac_add_options --disable-optimize
ac_add_options --disable-jemalloc
ac_add_options --enable-valgrind

Comment 1

[Robert Sayre]

taking for now, may reassign.

Comment 2

[Nicholas Nethercote [inactive]]

Jesse, can you still reproduce these?  I haven't seen such leaks when I've run Memcheck on the browser recently.

Comment 3

[Jesse Ruderman]

Yes, I can still reproduce these using Firefox trunk + Valgrind trunk.

I have to load a page to trigger the bug, but a blank page such as http://www.squarefree.com/temp2/ is enough to trigger the leak.

Comment 4

[Nicholas Nethercote [inactive]]

One relevant thing:  all of those sqlite reports are for "possibly lost" memory, which means that Valgrind found a pointer into the block, but the pointer wasn't to the start of the block.  SQLite allocations all go through this function:

static void *sqlite3MemMalloc(int nByte){
  sqlite3_int64 *p;
  assert( nByte>0 );
  nByte = ROUND8(nByte);
  p = malloc( nByte+8 );
  if( p ){
    p[0] = nByte;    // !!!
    p++;             // !!!
  }else{
    testcase( sqlite3GlobalConfig.xLog!=0 );
    sqlite3_log(SQLITE_NOMEM, "failed to allocate %u bytes of memory", nByte);
  }
  return (void *)p;
}

Note the lines marked with "!!!" -- this function always allocates 8 bytes more than asked for and stores the size in the first 8 bytes.  This is so the block size can be determined with sqlite3MemSize().  If this function did not do that, all those "possibly lost" blocks reported by Valgrind would not be reported, because they would be "still reachable".  So this isn't nearly as bad as it first looks.  In fact, it might even be worth closing this bug.

Hmm, it looks like if you #define SQLITE_MEMDEBUG then a different allocator is used, one which stores the size separately.  Seems like this would be worth turning on when --enable-valgrind is specified.  Looks like db/sqlite3/src/Makefile.in and configure.in would need changing as part of this change.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #4)
> 
> Hmm, it looks like if you #define SQLITE_MEMDEBUG then a different allocator
> is used, one which stores the size separately.  Seems like this would be
> worth turning on when --enable-valgrind is specified.

Nope;  turns out that with that allocator, even more stuff gets jammed in at the start of the block, so the same problem remains.

The only way I can see around this problem is to add a new client request to Memcheck, something like this:

  VALGRIND_BLOCK_POINTER(p, 8)

which means "treat any pointer that points to p+8 as if it points to p+0".  The offset could be stored in Valgrind's MC_Chunk struct, but that would require making that struct bigger, which is a problem.

Comment 6

[Jesse Ruderman]

How about adding a way to suppress "possibly lost" leaks without also suppressing "definitely lost" leaks?

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> How about adding a way to suppress "possibly lost" leaks without also
> suppressing "definitely lost" leaks?

That's also a possibility.  We currently just have the "Memcheck:Leak" category, which matches all leak report kinds.  We'd need to keep that for backwards compatibility, but we could introduce "DefiniteLeak", "PossibleLeak", "IndirectLeak", "StillReachable" sub-categories.

Comment 8

[Julian Seward [:jseward]]

(In reply to comment #7)
> (In reply to comment #6)
> > How about adding a way to suppress "possibly lost" leaks without also
> > suppressing "definitely lost" leaks?
> 
> That's also a possibility.

Seems plausible to me.  Shall I do it, or does it no longer seem on
reflection like a good solution?

Comment 9

[Nicholas Nethercote [inactive]]

Seems worth doing.

Comment 10

[Nicholas Nethercote [inactive]]

I don't think this is relevant any more, because we're using --show-possibly-lost=no.