Skip to content

Loading…

fix a few memory leakage issues #880

Closed
wants to merge 2 commits into from

4 participants

@ccding

Hi this pull request is to fix the memory leakage issues, while cleanup the trailing blank spaces in the source code. Thanks

@bwright bwright commented on the diff
deps/hiredis/sds.c
@@ -294,7 +294,11 @@ sds *sdssplitlen(char *s, int len, char *sep, int seplen, int *count) {
#ifdef SDS_ABORT_ON_OOM
if (tokens == NULL) sdsOomAbort();
#endif
- if (seplen < 1 || len < 0 || tokens == NULL) return NULL;
@bwright
bwright added a note

My only concern is that, under the definition of free from the standard:

Description
The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs.

So theoretically is it not more verbose then say:

if (seplen < 1 || len < 0 || tokens == NULL) {
    free(tokens);
    return NULL;
}

Under the assumption that the tokens need to be freed here.

@ccding
ccding added a note

Technically your code acts exactly same as my code, but I am not sure what's the author's tastes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwright bwright commented on the diff
deps/jemalloc/test/thread_arena.c
@@ -15,7 +15,7 @@
p = malloc(1);
if (p == NULL) {
malloc_printf("%s(): Error in malloc()\n", __func__);
- return (void *)1;
@bwright
bwright added a note

Seems valid based on reading the function definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwright bwright commented on the diff
deps/jemalloc/test/thread_arena.c
@@ -23,7 +23,7 @@
sizeof(main_arena_ind)))) {
malloc_printf("%s(): Error in mallctl(): %s\n", __func__,
strerror(err));
- return (void *)1;
@bwright
bwright added a note

Appears to be a valid case for a free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwright bwright commented on the diff
deps/jemalloc/test/thread_arena.c
@@ -31,11 +31,17 @@
0))) {
malloc_printf("%s(): Error in mallctl(): %s\n", __func__,
strerror(err));
- return (void *)1;
@bwright
bwright added a note

Appears to be a valid case for a free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwright bwright commented on the diff
deps/jemalloc/test/thread_arena.c
((10 lines not shown))
return (NULL);
+
@bwright
bwright added a note

Are gotos really the best solution for this?

@bwright
bwright added a note

Not against them, but adds a lot of arbitrator logic. If p is null and you free it nothing is going to happen so both lines can safely be executed.

@ccding
ccding added a note

Sure, personally I don't like goto, but I saw the author uses it in this way.

The free(NULL) issue I will do as your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bwright
  • Trailing space code seems great works well.
  • Memory leak code seems fine, but a bit verbose, maybe we can simplify it?
  • Some minor verbosity in sds.c, you have C standard guarantees on free(NULL).

All in all a cool patch, might want to comment or take into account what I suggested. Get back to me.

@badboy

I don't think jemalloc should be patched here (if necessary report this upstream and fix it there)

Plus that's a whole lot of whitespace-changes. Better keep them in a seperate pull-request.

@ccding

ok, I will do the whitespace one separately.

@ccding

what do you mean about the jemalloc issue? p is a local variable, if we don't free it here, it will be leaked forever.

@ccding ccding referenced this pull request
Closed

remove trailing white space #897

@badboy

The code in deps/ are dependencies. Therefore bugs there should be reported to the corresponding project, not to redis. So see jemalloc for issues with that.
From time to time and when needed antirez will pull in the actual jemalloc-code.

@ccding

ok, Thanks.

@ccding

so the fix memory leakage of variable token commit can be merged, and igore fix memory leakage of variable p?

@antirez
Owner

Hello, sds.c in Redis is already correct, it is the hiredis lib that has an old version. It is probably better to provide a pull request to hiredis that uses the new sds.c (from Redis) instead, that contains other changes as well.

All the others are about jemalloc test code, so nothing can be merged here. Thanks anyway for the pull request. Closing.

@antirez antirez closed this
@ccding ccding deleted the unknown repository branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 20, 2013
  1. @ccding
  2. @ccding

    fix memory leakage of variable p

    ccding committed
This page is out of date. Refresh to see the latest.
Showing with 13 additions and 4 deletions.
  1. +4 −1 deps/hiredis/sds.c
  2. +9 −3 deps/jemalloc/test/thread_arena.c
View
5 deps/hiredis/sds.c
@@ -294,7 +294,10 @@ sds *sdssplitlen(char *s, int len, char *sep, int seplen, int *count) {
#ifdef SDS_ABORT_ON_OOM
if (tokens == NULL) sdsOomAbort();
#endif
- if (seplen < 1 || len < 0 || tokens == NULL) return NULL;
@bwright
bwright added a note

My only concern is that, under the definition of free from the standard:

Description
The free function causes the space pointed to by ptr to be deallocated, that is, made available for further allocation. If ptr is a null pointer, no action occurs.

So theoretically is it not more verbose then say:

if (seplen < 1 || len < 0 || tokens == NULL) {
    free(tokens);
    return NULL;
}

Under the assumption that the tokens need to be freed here.

@ccding
ccding added a note

Technically your code acts exactly same as my code, but I am not sure what's the author's tastes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (seplen < 1 || len < 0 || tokens == NULL) {
+ free(tokens);
+ return NULL;
+ }
if (len == 0) {
*count = 0;
return tokens;
View
12 deps/jemalloc/test/thread_arena.c
@@ -15,7 +15,7 @@ je_thread_start(void *arg)
p = malloc(1);
if (p == NULL) {
malloc_printf("%s(): Error in malloc()\n", __func__);
- return (void *)1;
@bwright
bwright added a note

Seems valid based on reading the function definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ goto label_return;
}
size = sizeof(arena_ind);
@@ -23,7 +23,7 @@ je_thread_start(void *arg)
sizeof(main_arena_ind)))) {
malloc_printf("%s(): Error in mallctl(): %s\n", __func__,
strerror(err));
- return (void *)1;
@bwright
bwright added a note

Appears to be a valid case for a free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ goto label_return;
}
size = sizeof(arena_ind);
@@ -31,11 +31,16 @@ je_thread_start(void *arg)
0))) {
malloc_printf("%s(): Error in mallctl(): %s\n", __func__,
strerror(err));
- return (void *)1;
@bwright
bwright added a note

Appears to be a valid case for a free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ goto label_return;
}
assert(arena_ind == main_arena_ind);
+ free(p);
return (NULL);
+
@bwright
bwright added a note

Are gotos really the best solution for this?

@bwright
bwright added a note

Not against them, but adds a lot of arbitrator logic. If p is null and you free it nothing is going to happen so both lines can safely be executed.

@ccding
ccding added a note

Sure, personally I don't like goto, but I saw the author uses it in this way.

The free(NULL) issue I will do as your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+label_return:
+ free(p);
+ return (void *)1;
}
int
@@ -75,6 +80,7 @@ main(void)
je_thread_join(threads[i], (void *)&ret);
label_return:
+ free(p);
malloc_printf("Test end\n");
return (ret);
}
Something went wrong with that request. Please try again.