Fixed all memory leaks and almost all undefined behaviour #4025
Conversation
for good this time... probably
| (void)argc; | ||
| (void)argv; | ||
|
|
||
| if (!buffer || !out || !roundtrip || !cctx || !dctx) { | ||
| fprintf(stderr, "Allocation failure\n"); | ||
| return 1; | ||
| _exit_code = 1; | ||
| goto cleanup; |
| @@ -1273,7 +1273,6 @@ static int createBuffers(buffers_t* buff, const char* const * const fileNamesTab | |||
| f = fopen(fileNamesTable[n], "rb"); | |||
| if (f==NULL) { | |||
| DISPLAY("impossible to open file %s\n", fileNamesTable[n]); | |||
| fclose(f); | |||
|
The first list of changes, detailed in the summary and present in the first commit, looks mostly good to me (with the exception of But then, there is a second list of changes, named "fixed ISO C incompatibility" in commits 2 and 3, which are a bit more concerning and would deserve some scrutiny. And strangely, while they appear in the commit timeline, I don't see them in the GitHub PR comparison view. |
zlibWrapper/gzwrite.c
Outdated
| @@ -64,6 +64,8 @@ local int gz_init(gz_statep state) { | |||
| strm->next_out = state.state->out; | |||
| state.state->x.next = strm->next_out; | |||
| } | |||
|
|
|||
| free(state.state); | |||
There was a problem hiding this comment.
That one is weird.
I'm not even sure what's going on in this code.
One of the first actions in this function is : state.state->in = (unsigned char*)malloc(state.state->want << 1);,
which presumes that state.state is already allocated (should probably be asserted), before entering the function,
which means that, something else has allocated state.state, and therefore something else is in charge of freeing it.
I don't see how it could be good to free it here.
There was a problem hiding this comment.
Apologies, You're right. That was an oversight from me.
I'll fix this tomorrow morning... Again, sorry for the inconvenience
Yeah, about those, |
doc/educational_decoder/harness.c
Outdated
| @@ -50,6 +50,7 @@ static buffer_s read_file(const char *path) | |||
|
|
|||
| fclose(f); | |||
| buffer_s const b = { ptr, size }; | |||
| free(ptr); | |||
There was a problem hiding this comment.
This change is incorrect :
the whole point of this function is to return a populated buffer, passed as a member of the buffer_s structure, effectively transferring ownership to the caller of the function (which will have to free it later, using the provided freeBuffer() function).
Maybe this could be documented if it's not clear enough...
zlibWrapper/examples/minigzip.c
Outdated
| @@ -234,7 +234,7 @@ int gzwrite _Z_OF((gzFile, const void *, unsigned)); | |||
|
|
|||
| int gzwrite(gzFile gz, const void *buf, unsigned len) { | |||
| z_stream *strm; | |||
| unsigned char out[BUFLEN]; | |||
| unsigned char out[BUFLEN] = 0; | |||
There was a problem hiding this comment.
God damn, I'm absent-minded...
Sorry I made so much mistakes in something that's supposed to fix stuff...
Will fix this though, thanks for noticing
fixed where i made it to init with just the first one being set to 0
|
There is another bug in this PR, This formulation is erroneous: |
Misc
fclose(f)line attests/paramgrill.cincreateBuffersfunction, since iffcouldn't initialize properly, there's no need to close it either.tests/regression/result.cinresult_get_error_string(result_t result)function, just in case.MemLeaks
Fixed memory leaks/possible memory leaks at:
doc/educational_decoder/harness.cduringread_filefunction.tests/bigdict.cnow goes to cleanup if condition(!buffer || !out || !roundtrip || !cctx || !dctx)is met, instead of simply returning 1.zlibWrapper/gzwrite.cnow frees state.state before returning from the function.