kernel: fix memory leaks in lantiq

[draane]

Add calls to free for the variables buf_orig and buf.
Those two variables were never freed, causing possible
memory leaks.

[neheb]

To simplify, you could use GCC's cleanup attribute: http://echorand.me/site/notes/articles/c_cleanup/cleanup_attribute_c.html

[draane]

To simplify, you could use GCC's cleanup attribute: http://echorand.me/site/notes/articles/c_cleanup/cleanup_attribute_c.html

Oh that is nice, I didn't know about it. So, should I put a single free in the cleanup attribute and remove all the other ones?

[ynezz]

To simplify, you could use GCC's cleanup attribute

@neheb In kernel?

So, should I put a single free in the cleanup attribute

I would rather use common kernel goto idiom:

  if (!foo)
    goto free_buf;

  if (!bar)
    goto free_buf_orig;

  return 0;

free_buf_orig:
  free(buf_orig);
free_buf:
  free(buf);

  return -1;

[neheb]

To simplify, you could use GCC's cleanup attribute

@neheb In kernel?

This looks like a standalone utility.

[neheb]

To simplify, you could use GCC's cleanup attribute: http://echorand.me/site/notes/articles/c_cleanup/cleanup_attribute_c.html

Oh that is nice, I didn't know about it. So, should I put a single free in the cleanup attribute and remove all the other ones?

IIRC, you create a static function that takes a double pointer as an argument that just calls free. It makes all those if statements useless as GCC handles cleaning up automatically.

It's a way to do C++ destructors in C (similar but not quite the same). systemd does this all over the codebase.

[draane]

@neheb I changed the file as you suggested, adding the cleanup attribute.
@ynezz are the changes ok for you?

[neheb]

That should be %zu I believe.

[draane]

The original code used %d but I can change that.
Also should I keep the new if I created? Or just keep it as it was originally?

[neheb]

I don't see why not.

[draane]

Ok 👍
I'm gonna change the string format

[adschm]

@neheb Can you have another look at this?

[neheb]

Seems fine.

[adschm]

@ynezz Would you review this again?

[adschm]

@ynezz Ping.

[neheb]

ping @hauke

[CodeFetch]

Looks good to me.

@hauke @ynezz Is it okay?

[adschm]

Commit title prefix should be "ltq-vdsl-fw:" ...

[CodeFetch]

@draane Ping...

[draane]

@adschm @CodeFetch I just changed the commit title

[tomjnixon]

I could be wrong, but I think this patch introduces undefined behaviour. If main returns before the buffers are allocated, then free_buffer will be call free with an uninitialised pointer, which could point anywhere. Running this in valgrind shows the problem:

#include <stdlib.h>

void free_mem(void **mem) { free(*mem); }

int main(int argc, char **argv) {
  void *mem __attribute__((__cleanup__(free_mem)));

  return 0;
}

This is fixable by either initialising the pointers to NULL when they are declared, or declare and allocate on the same line.

Personally I don't think this is a memory leak, as the memory is freed when the program exits.

[mkresin]

The author of the code seconds the last comment:

imho #2678 is a non-problem. the extractor runs once and all memory is freed anyway when the execution ends.

Unlikely that the code is required or will be ever merged. Closing here as we are talking about "possible memory leaks" without a proof.