Closed Bug 573688 Opened 10 years ago Closed 6 years ago

sqlite leaks a bunch

Categories

(Toolkit :: Storage, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED INVALID

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: memory-leak, valgrind)

Attachments

(1 file)

Attached file 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
Depends on: 639408
Assignee: nobody → sayrer
taking for now, may reassign.
Jesse, can you still reproduce these?  I haven't seen such leaks when I've run Memcheck on the browser recently.
No longer blocks: mlk-fx5+
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.
Assignee: sayrer → nobody
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.
(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.
How about adding a way to suppress "possibly lost" leaks without also suppressing "definitely lost" leaks?
(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.
Blocks: 639408
No longer depends on: 639408
(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?
Seems worth doing.
I don't think this is relevant any more, because we're using --show-possibly-lost=no.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.