Skip to content

Commit

Permalink
Stop memory leak in NDArrayPool::alloc
Browse files Browse the repository at this point in the history
When the caller supply a new memory buffer in the pData argument,
the old pArray->pData pointer need to be freed or it will leak as the
new pointer simply overwrite the old one, losing the only reference
to the original buffer.
We can safely free the old pArray->pData buffer as the pArray is
either a brand new one with pData==NULL or one off the NDArrayPool
free-list.
  • Loading branch information
ulrikpedersen committed Jan 30, 2014
1 parent 160aa8c commit 3e29106
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions ADApp/ADSrc/NDArrayPool.cpp
Expand Up @@ -111,6 +111,8 @@ NDArray* NDArrayPool::alloc(int ndims, size_t *dims, NDDataType_t dataType, size
if (pArray) {
/* If the caller passed a valid buffer use that, trust that its size is correct */
if (pData) {
// If the caller supply a new memory buffer, first free the old one.
if (pArray->pData) { free(pArray->pData); }
pArray->pData = pData;
} else {
/* See if the current buffer is big enough */
Expand Down

4 comments on commit 3e29106

@ulrikpedersen
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment emailed from Jon Thompson:
(if I’m understanding the problem correctly) are you sure the pointer you are deleting can never be the external memory? Might it be that the NDArray was earlier created with external memory, been released and then re-used? It might be best to record the fact that the memory is externally allocated and only delete when not. In fact there’s another case here where the re-use is not with external memory; new internal memory should be allocated rather than re-using the old external memory.

@ulrikpedersen
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point Jon, and I have considered it:

If the user has created the NDArray with external memory, used it, and then released it - how would the user have known that it was safe to re-use that block of memory? This NDArray may still be in the input-queue of any 'downstream' plugin (some plugins have queues of 1000s of NDArrays, keeping a hold of it for several minutes). In the current NDArray and NDArrayPool designs, there is no way for an external user to know whether or not the NDArray is still queued somewhere (the reference count is strictly private).

So I think that (in the current design) any externally allocated memory given to an NDArray, belongs to the NDArray as the NDArray does not notify when it is done using it. This may not have been documented or even voiced out loud, but that is what I read from the code...

I agree that this limits the use of external memory management quite significantly - and that is why my next commit: db7199a provide an OO mechanism to notify the user of the NDArrayPool that an NDArray with externally allocated memory is about to be put on the free-list. The user can then decide whether to regain ownership of that memory pointer or free it or ignore the notification alltogether (the NDArray will continue to own it). I feel this allow for any possible use-case without breaking backwards compatibility and without making life difficult for developers.

Regarding your comment about re-allocating internal (to NDArray/NDArrayPool) memory: I think this is a slightly different issue to this - although it would probably touch on some of the same bits of code. Feel free to make another branch and hack away (but keep in mind backwards compatibility!).

@ajgdls
Copy link

@ajgdls ajgdls commented on 3e29106 Jan 31, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Jon that if the memory supplied to the NDArray is allocated externally then the NDArray cannot really presume it has the right to ever free that memory. If a third party library allocates some piece of memory then I expect it will always try to free it at some point (or it should to avoid memory leaks). This is why I think the callback is a good idea. If the NDArry records that it received allocated memory externally then after it has made the callback to say it's complete it can delete the pointer to that memory, happy in the knowledge that the external library should have taken care of its own cleanup.

@ulrikpedersen
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alan, Thanks for the comment!

So the next commit db7199a actually does what you ask here: it notifies the user that the NDArray is about to be put on the free-list. This gives the user 3 options regarding what to do with that block of memory and the NDArray it came from:

  • The user can completely ignore the callback - this is the backwards compatible way where the memory now belongs to the NDArray - and it can potentially free it or even lose the pointer (i.e. leak) if another pData pointer is supplied!
  • The user can catch the callback (i.e. implement the callback method) and re-claim the memory buffer by returning a NULL pointer back to the NDArray.
  • The user can catch the callback and return another allocated buffer (of the same size) back to the NDArray.

Regardless of what the user decide to do - the fix in this particular commit is still required to avoid a leak!

Cheers,
Ulrik

Please sign in to comment.