Buffer leak #164
Buffer leak #164
Conversation
ext/nio4r/bytebuffer.c
Outdated
| @@ -84,6 +84,7 @@ static void NIO_ByteBuffer_gc_mark(struct NIO_ByteBuffer *buffer) | |||
|
|
|||
| static void NIO_ByteBuffer_free(struct NIO_ByteBuffer *buffer) | |||
| { | |||
| xfree(buffer->buffer); | |||
There was a problem hiding this comment.
The indentation is not aligned. Please use 4 spaces
|
Several of the tests are now segfaulting, e.g. |
ext/nio4r/bytebuffer.c
Outdated
| @@ -93,6 +94,7 @@ static VALUE NIO_ByteBuffer_initialize(VALUE self, VALUE capacity) | |||
| struct NIO_ByteBuffer *buffer; | |||
| Data_Get_Struct(self, struct NIO_ByteBuffer, buffer); | |||
|
|
|||
| buffer->buffer = NULL; | |||
There was a problem hiding this comment.
Would probably be safer to do this in allocate
| @@ -75,6 +75,7 @@ void Init_NIO_ByteBuffer() | |||
| static VALUE NIO_ByteBuffer_allocate(VALUE klass) | |||
| { | |||
| struct NIO_ByteBuffer *bytebuffer = (struct NIO_ByteBuffer *)xmalloc(sizeof(struct NIO_ByteBuffer)); | |||
| bytebuffer->buffer = NULL; | |||
There was a problem hiding this comment.
And it might be good to just go ahead and zero the whole structure here, so long as you're changing stuff 😉
There was a problem hiding this comment.
And I thought it was going to be an easy patch :) I'll do it tomorrow as I come back to the laptop.
There was a problem hiding this comment.
Thanks for doing this! This code was from a GSoC project, but it seems I missed this while reviewing it and it really should be properly initialized at the time it's allocated.
Another approach that ensures the buffer is properly allocated would be to malloc a 0 byte buffer, then realloc it once initialize is called. This seems like the safest option to me, although not necessarily the most efficient.
There was a problem hiding this comment.
Using realloc() in initialize would also ensure if initialize is called repeatedly from Ruby-land no memory would be leaked.
|
Ok, well went ahead and merged this because it's an improvement over where it was before. If you'd like to circle back on the other things I suggested that'd be great, otherwise I might try to tackle them. |
|
Hey @UpeksheJay, any interest in fixing up some of the stuff we discussed above? |
|
@tarcieri since it's already merged, I'll leave it, seems to be quick chore that can be done before issuing a release. I assume I'm the first one trying to use byte buffers is an actual project (?). Is the Java version being used in production somewhere? |
ByteBuffer leaks memory, because the char buffer is left behind.