Skip to content
Permalink
Browse files

simplify recent bug fix

  • Loading branch information...
shawnl committed Apr 14, 2019
1 parent b49b00e commit bbfddaa66e85913abce5f978c6b22a11d37326bc
Showing with 3 additions and 5 deletions.
  1. +3 −5 src/stringmap.c
@@ -48,15 +48,13 @@ stringmap_t *stringmap_load(const char *filename, int numFinalWordsToMatch)
char buf[2*PATH_MAX];
int n;

fp = fopen(filename, "r");
if (!fp)
return NULL;
result = calloc(1, sizeof(*result));
if (!result)

This comment has been minimized.

Copy link
@Oleh-Kravchenko

Oleh-Kravchenko Apr 14, 2019

Contributor

If calloc() fail, fp will be lost

This comment has been minimized.

Copy link
@shawnl

shawnl Apr 14, 2019

Author Collaborator

oh yeah. good point.

return NULL;
result->numFinalWordsToMatch = numFinalWordsToMatch;
fp = fopen(filename, "r");
if (!fp) {
free(result);
return NULL;
}
n=0;
while (fgets(buf, sizeof(buf), fp))
n++;

8 comments on commit bbfddaa

@Oleh-Kravchenko

This comment has been minimized.

Copy link
Contributor

Oleh-Kravchenko replied Apr 14, 2019

Congratulations! You just add new possible resource leak :)

You should add fclose(fp) after line 55.

    if (!result) {
        fclose(fp);
        return NULL;
    }
@shawnl

This comment has been minimized.

Copy link
Collaborator Author

shawnl replied Apr 14, 2019

Are you doing this as part of a class?

@shawnl

This comment has been minimized.

Copy link
Collaborator Author

shawnl replied Apr 14, 2019

Because the code quality of distcc is not good, and never was. There is another malloc right below this that is not checked, and the strv stuff is insane, because it doesn't use realloc() but rather hopes that the allocation is good enough.

@Oleh-Kravchenko

This comment has been minimized.

Copy link
Contributor

Oleh-Kravchenko replied Apr 15, 2019

Are you doing this as part of a class?
No, I just distcc user. Try to improve favorite tool :)

Because the code quality of distcc is not good, and never was. There is another malloc right below this that is not checked, and the strv stuff is insane, because it doesn't use realloc() but rather hopes that the allocation is good enough.

Yes, I saw it. It not memory leak, program will fail with segfault at

result->map[n].value = strdup(buf);

@shawnl

This comment has been minimized.

Copy link
Collaborator Author

shawnl replied Apr 15, 2019

No, I just distcc user. Try to improve favorite tool :)

It really should be re-written. I was trying to work on supporting TLS with curve25519 and this tls extension https://datatracker.ietf.org/doc/rfc7250/?include_text=1, but realized that distcc should just be re-writen. It is not excessively complex an application, especially if you do not support pump mode (which I do not use).

Google was using it (and contributed pump mode), and then they re-wrote it, but have not released their rewrite as free software.

@Oleh-Kravchenko

This comment has been minimized.

Copy link
Contributor

Oleh-Kravchenko replied Apr 15, 2019

No, I just distcc user. Try to improve favorite tool :)

It really should be re-written. I was trying to work on supporting TLS with curve25519 and this tls extension https://datatracker.ietf.org/doc/rfc7250/?include_text=1, but realized that distcc should just be re-writen. It is not excessively complex an application, especially if you do not support pump mode (which I do not use).

Google was using it (and contributed pump mode), and then they re-wrote it, but have not released their rewrite as free software.

Let's rewrite it! :) Do you have any ideas how it should be?

PS:
May we should move our discussion to email? (or maybe IM)

@shawnl

This comment has been minimized.

Copy link
Collaborator Author

shawnl replied Apr 15, 2019

I don't have time, but it would probably make a great project for you.

Are you on IRC. I am on freenode as scientes and am in the #distcc channel.

@TafThorne

This comment has been minimized.

Copy link
Collaborator

TafThorne replied Apr 18, 2019

Please sign in to comment.
You can’t perform that action at this time.