Opened 9 years ago

Closed 9 years ago

#6378 closed bug (fixed)

Block Cache Memory Leak

Reported by: jvff Owned by: axeld
Priority: normal Milestone: R1
Component: System/Kernel Version: R1/alpha2
Keywords: block cache, memory leak Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

cache_detach_sub_transaction was overwritting the block->original_data pointer without freeing the previous data it pointed to.

Attachments (1)

block_cache_leak.patch (561 bytes ) - added by jvff 9 years ago.
This patch frees the data block, but it assumes that the pointer value won't be overwritten by it's own value (otherwise it will point to a freed area). Also, the patch was generated from trunk/src/system/ because I had other files changed that weren't relevant to the patch.

Download all attachments as: .zip

Change History (7)

by jvff, 9 years ago

Attachment: block_cache_leak.patch added

This patch frees the data block, but it assumes that the pointer value won't be overwritten by it's own value (otherwise it will point to a freed area). Also, the patch was generated from trunk/src/system/ because I had other files changed that weren't relevant to the patch.

comment:1 by jvff, 9 years ago

Has a Patch: set

comment:2 by jvff, 9 years ago

I ran block_cache.cpp through grep to find where stuff was assigned (ie. grep "<src>.* = ") and I reached the following dependancy:

  • original_data is either NULL, returned from Allocate or copied from parent_data (where the leak originally occurred);
  • parent_data is either NULL, returned from Allocate or copied from current_data;
  • current_data is returned from Allocate.

This (initially) leads me to the conclusion that a "false overwrite" doesn't happen, which means the patch is (in theory) safe. However, it would be better if someone with more knowledge of the code could confirm this.

in reply to:  description comment:3 by bonefish, 9 years ago

Replying to jvff:

Also, the patch was generated from trunk/src/system/ because I had other files changed that weren't relevant to the patch.

Please know that you can pass the paths which you want to include in the patch to svn diff. There's no reason for creating a patch from a subdirectory.

comment:4 by axeld, 9 years ago

Owner: changed from nobody to axeld
Status: newin-progress

comment:5 by axeld, 9 years ago

The code in cache_detach_sub_transaction() is really pretty much broken, however, your patch only works around the actual issue. Thanks for looking into it, though!

I'm currently working on fixing the bug for real, and add a bit more documentation that should ease understanding the code.

comment:6 by axeld, 9 years ago

Component: File SystemsSystem/Kernel
Resolution: fixed
Status: in-progressclosed

Fixed for real in hrev37899.

Note: See TracTickets for help on using tickets.