4 ms·

Where are the _obvious_ memory leaks?

by cremno 12y ago

revelation 12y ago
http://cgit.freedesktop.org/systemd/systemd/tree/src/import/import-dck.c?id=72648326ea6d3e68cdb0b5890df737047d031a41#n1140 http://cgit.freedesktop.org/systemd/systemd/tree/src/import/... The liberal use of completely unportable cleanup attributes shifts preventing memory leaks to the equally intractable problem of tracking stack allocated variable assignments, with an extra helping of double frees. I won't even comment on the show of horrors that is util: http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c?id=72648326ea6d3e68cdb0b5890df737047d031a41 http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/...
- cremno 12y ago
  I don't like the usage of that attribute either (but if that's their strategy, fine by me.), but I still don't see where the obvious memory leaks are. Your first link links to a line with a strdup() call, but the corresponding free() can be found in L168.
  - revelation 12y ago
    Sorry, I linked to line 1140, but it doesn't highlight that and can't scroll to it as it is towards the end. I did just check again and realize that the single 'a' at the strndupa means it's using alloca, which is of course similarly unportable and has a whole slew of other problems, all of them intractable (did you pass that pointer to something that will use it later on? stack overflows?). So hey, not a memory leak! Rejoice! It's just similarly broken.
    - cremno 12y ago
      Oh, I am the one that has to be sorry! I didn't check the URL, even though I am aware of that issue with cgit (no line highlighting). Yeah, using alloca() (in any form, incl. VLA) isn't good style. At least it's bounded, since filename_is_valid() checks the length. But it uses FILENAME_MAX, which could be too large for the stack.
      - jacquesm 12y ago
        > But it uses FILENAME_MAX, which could be too large for the stack. That's extremely unlikely. This is definitely not pretty or good quality C code by any standard but let's not start spreading wrong information. The stack overflowing because you use alloc for something of FILENAME_MAX length is just as likely as a malloc call running out of memory. After all, the heap grows 'up' and the stack grows 'down'. Other than that the mechanisms are roughly (very roughly, ok) identical. The only time when you have to be extremely careful with alloca is when you use it in functions that might be called in a recursive manner. But then you're playing with fire anyway if you do not have hard upper bounds on the depth of your recursion.
    - to3m 12y ago
      Which operating systems does systemd support? Few that don't have gcc as their official compiler, I would have thought?
      - digi_owl 12y ago
        Linux. Poettering has in the past stated to take the book on Linux and Unix programming, and toss the sections on Unix.
    - tomegun 12y ago
      Similarly broken how? Just because strdupa is not magical, does not mean it has its valid uses. In particular, the problem you allude to (knowing the scope of your memory) is something you would have to get right also if you use heap-allocated memory (unless you just leak everything of course, but I think we have established that that's not something we want to do).
- jacquesm 12y ago
  That sort of code is the reason why C has become a dirty word.
- EmanueleAina 12y ago
  Note that unfreed results of strdupa(), despite having "dup" in the name, does not result in memory leaks. The "a" in the name means it uses alloca(), thus the memory comes from the stack and it's properly released on function return. It's also probably slightly faster than malloc() since it just moves the stack pointer. Before jumping to conclusions, please verify what the code really does. C is a complex beast and what's "obvious" may not be such. Relying on such niceties (as weird as it may seem, to a C programmer those are niceties) is what you gain if you are willing to trade them for portability (strndupa() is a GNU extension).
  - jacquesm 12y ago
    This is 100% correct. The 'a' stands for 'automatic', just like the variables allocated on the stack are 'automatic' variables. When the stack frame is unwound when the function returns the allocation is undone.
- tomegun 12y ago
  Hm, this criticism seems hardly fair. systemd is explicitly unportable (it uses glibc/gcc/linux-specific features liberally), this is not some sign of sloppy coding, but an explicit design decision as codified in the CODING_STYLE file. Preventing memory leaks and tracking stack allocated variable assignment are not comparable at all. In the vast majority of cases the tracking of stack allocated variable assignments is entirely trivial. The problem with double freeing is also very hard to run into in practice. Getting a memory-leak when you don't use _cleanup_ is all too common on the other hand. Basically, as soon as there are more than one branch in your function you either have to use ugly goto's or remember to free on every branch, in practice either way you are just implementing the _cleanup_ macros manually all over the place (with all the bugs that implies). Why don't you point to some code that would have been better without the use of the macros? It seems you think util.c is a god source of examples...

Where are the _obvious_ memory leaks?

by cremno 12y ago

Where are the _obvious_ memory leaks?

http://cgit.freedesktop.org/systemd/systemd/tree/src/import/import-dck.c?id=72648326ea6d3e68cdb0b5890df737047d031a41#n1140 http://cgit.freedesktop.org/systemd/systemd/tree/src/import/... The liberal use of completely unportable cleanup attributes shifts preventing memory leaks to the equally intractable problem of tracking stack allocated variable assignments, with an extra helping of double frees. I won't even comment on the show of horrors that is util: http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/util.c?id=72648326ea6d3e68cdb0b5890df737047d031a41 http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/...

cremno 12y ago
I don't like the usage of that attribute either (but if that's their strategy, fine by me.), but I still don't see where the obvious memory leaks are. Your first link links to a line with a strdup() call, but the corresponding free() can be found in L168.
- revelation 12y ago
  Sorry, I linked to line 1140, but it doesn't highlight that and can't scroll to it as it is towards the end. I did just check again and realize that the single 'a' at the strndupa means it's using alloca, which is of course similarly unportable and has a whole slew of other problems, all of them intractable (did you pass that pointer to something that will use it later on? stack overflows?). So hey, not a memory leak! Rejoice! It's just similarly broken.
  - cremno 12y ago
    Oh, I am the one that has to be sorry! I didn't check the URL, even though I am aware of that issue with cgit (no line highlighting). Yeah, using alloca() (in any form, incl. VLA) isn't good style. At least it's bounded, since filename_is_valid() checks the length. But it uses FILENAME_MAX, which could be too large for the stack.
    - jacquesm 12y ago
      > But it uses FILENAME_MAX, which could be too large for the stack. That's extremely unlikely. This is definitely not pretty or good quality C code by any standard but let's not start spreading wrong information. The stack overflowing because you use alloc for something of FILENAME_MAX length is just as likely as a malloc call running out of memory. After all, the heap grows 'up' and the stack grows 'down'. Other than that the mechanisms are roughly (very roughly, ok) identical. The only time when you have to be extremely careful with alloca is when you use it in functions that might be called in a recursive manner. But then you're playing with fire anyway if you do not have hard upper bounds on the depth of your recursion.
  - to3m 12y ago
    Which operating systems does systemd support? Few that don't have gcc as their official compiler, I would have thought?
    - digi_owl 12y ago
      Linux. Poettering has in the past stated to take the book on Linux and Unix programming, and toss the sections on Unix.
  - tomegun 12y ago
    Similarly broken how? Just because strdupa is not magical, does not mean it has its valid uses. In particular, the problem you allude to (knowing the scope of your memory) is something you would have to get right also if you use heap-allocated memory (unless you just leak everything of course, but I think we have established that that's not something we want to do).
jacquesm 12y ago
That sort of code is the reason why C has become a dirty word.
EmanueleAina 12y ago
Note that unfreed results of strdupa(), despite having "dup" in the name, does not result in memory leaks. The "a" in the name means it uses alloca(), thus the memory comes from the stack and it's properly released on function return. It's also probably slightly faster than malloc() since it just moves the stack pointer. Before jumping to conclusions, please verify what the code really does. C is a complex beast and what's "obvious" may not be such. Relying on such niceties (as weird as it may seem, to a C programmer those are niceties) is what you gain if you are willing to trade them for portability (strndupa() is a GNU extension).
- jacquesm 12y ago
  This is 100% correct. The 'a' stands for 'automatic', just like the variables allocated on the stack are 'automatic' variables. When the stack frame is unwound when the function returns the allocation is undone.
tomegun 12y ago
Hm, this criticism seems hardly fair. systemd is explicitly unportable (it uses glibc/gcc/linux-specific features liberally), this is not some sign of sloppy coding, but an explicit design decision as codified in the CODING_STYLE file. Preventing memory leaks and tracking stack allocated variable assignment are not comparable at all. In the vast majority of cases the tracking of stack allocated variable assignments is entirely trivial. The problem with double freeing is also very hard to run into in practice. Getting a memory-leak when you don't use _cleanup_ is all too common on the other hand. Basically, as soon as there are more than one branch in your function you either have to use ugly goto's or remember to free on every branch, in practice either way you are just implementing the _cleanup_ macros manually all over the place (with all the bugs that implies). Why don't you point to some code that would have been better without the use of the macros? It seems you think util.c is a god source of examples...