5 ms·

Here's a trick that will actually help produce more secure and reliable programs. *outArg = myPtr; myPtr = NULL; free(aPtr); aPtr = NULL; Set your poi

by btrask 6y ago

Here's a trick that will actually help produce more secure and reliable programs. *outArg = myPtr; myPtr = NULL; free(aPtr); aPtr = NULL; Set your pointers to null when you free them! Set them to null when you transfer ownership! Stop leaving dangling pointers everywhere! Some people say they like dangling pointers because they want their program to crash if something is freed when they don't expect it to be. Good! Do this: assert(ptr); There are also many more tricks you can do once you start nulling pointers. You can use const to mark pointers that you don't own and thus can't free. You can check that buffers are all zero before you free them to catch memory leaks (this requires zeroing out other fields too of course). Please, null out your pointers and stop writing (most) use-after-free bugs!

saagarjha 6y ago
Your approach requires extra checks, though, which are easy to forget. Also, NULL is not guaranteed to be the stored as zeros, plus padding is going to make your life annoying.
- btrask 6y ago
  Well, dangling pointers are also easy to forget... Yes, it requires some discipline. Good code requires discipline, doesn't it? The trick of checking that buffers are zeroed is purely a debugging tool, so it's okay if it doesn't work on some platforms. And if you allocate with calloc(), the padding will be zeroed for you. It's actually very rare that you will have to call memset() with this technique.
  - saagarjha 6y ago
    > Good code requires discipline, doesn't it? This is like the most clichéd way of saying “my code has security vulnerabilities” that there is. I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things. > The trick of checking that buffers are zeroed is purely a debugging tool, so it's okay if it doesn't work on some platforms. Fair. > And if you allocate with calloc(), the padding will be zeroed for you. It might get unzeroed if you work with the memory.
    - btrask 6y ago
      All code is full of vulnerabilites. If you say your code isn't, then I'm sure it is. I just do the best I can to keep the error rate as low as possible. But it's a rate, and it's never zero. Also, it's not just about vulns in security-critical code. It's also about ordinary bugs. Why not be a little more careful? It won't hurt. > It might get unzeroed if you work with the memory. Maybe, but it isn't very common. I'm not sure when the C standard allows changing padding bytes, but in practice the compilers I've used don't seem to do it. And again, it's just a debugging aid, if it causes too much trouble on some platform, just turn it off.
      - saagarjha 6y ago
        It’s better to have automatic checks than rely on programmers being careful enough to remember to add them. For padding: this probably happens more on architectures that don’t do unaligned accesses very well.
        btrask 6y ago
        Help me out here, because I'm really trying to understand. Are you saying that dangling pointers that blow up if you double-free them is an "automatic check"? If not, what kind of automatic check are you talking about? If the extra code is really that bothersome, just use a macro or wrapper function.
        saagarjha 6y ago
        It's a much better situation than NULLing them out, because that hides bugs and makes tools like Address Sanitizer useless. A dangling pointer, when freed, will often throw an assert in your allocator; here's an example of how this looks like on my computer: $ clang -x c - #include <stdlib.h> int main(int argc, char **argv) { char *foo = malloc(10); free(foo); free(foo); } $ ./a.out a.out(14391,0x1024dfd40) malloc: *** error for object 0x11fe06a30: pointer being freed was not allocated a.out(14391,0x1024dfd40) malloc: *** set a breakpoint in malloc_error_break to debug Abort trap As you turn up your (automatic) checking this will be caught more and more often. Setting the pointer to NULL will silently hide the error as free(NULL) is a no-op and nothing will catch it. Thus, the suggestion here was 1. advocating adding additional code, which has historically been hard to actually do in practice, and 2. providing a suggestion that is generally worse.
- nsajko 6y ago
  > NULL is not guaranteed to be the stored as zeros Is that a real issue, though? > padding is going to make your life annoying Just memset?
  - huhtenberg 6y ago
    >> NULL is not guaranteed to be the stored as zeros > Is that a real issue, though? Of course, it's not, but that's one of those factoids that everyone learns at some point and feels like needing to rub it into everyone else's face assuming that these poor schmucks are as oblivious to it as they once were. A circle of life and all that.
    - saagarjha 6y ago
      Forgive me for encouraging the adoption of portable, compliant code to those who may not otherwise be aware of it. If you want to assume all the world’s an x86 that’s great but you should at least know what part of your code is going to be wrong elsewhere.
      - huhtenberg 6y ago
        Name a single platform where NULL is not 0.
  - tjalfi 6y ago
    AMD GPUs use a non-zero NULL pointer[0]. [0] https://reviews.llvm.org/D26196 https://reviews.llvm.org/D26196
    - nsajko 6y ago
      Interesting. And it isn't even always non-zero; sometimes it's 32-bit -1, sometimes it's 32-bit 0 and sometimes it's 64-bit 0: https://llvm.org/docs/AMDGPUUsage.html#address-spaces https://llvm.org/docs/AMDGPUUsage.html#address-spaces
- deleted 6y ago
  [deleted]
- spc476 6y ago
  NULL is required to be stored as all zeros on POSIX systems.
annilt 6y ago
Please don’t get me wrong but these precautions sound like you are sweeping problems under the carpet which will come out one day back again. It sounds like you have ownership issues in the design and trying to hide ‘possible future bugs’. Do you use sanitizers for use-after free bugs? I see many people still don’t use them even though sanitizers have become very good in the last 5-6 years
- btrask 6y ago
  It's defensive coding. Do you think defensive driving is 'sweeping problems under the carpet'? (It is, but it's still useful...) I use every tool at my disposal. Sanitizers, static analyzers... and also not leaving dangling pointers in the first place. Why would I do anything less? It doesn't cost anything except a little effort. Take a look at this recent HN link: https://www.radsix.com/dashboard1/ https://www.radsix.com/dashboard1/ . Look at all those use-after-free bugs. Even if it only happens 1% or 0.01% of the time... It's a huge class of bugs in C code. Why not take such a simple step?
  - annilt 6y ago
    If it works for you, then it is okay. It is not ‘a little effort’ for me to worry about someone else might use this pointer mistakenly, so I need to think about that all the time. It shifts my focus from problem solving to preventing future undefined behavior bugs. These bugs in the link, I don’t know C++, it is a big language which does a lot of things automatically, so it is already scary for me :) Maybe that is it, I write C server side code mostly(database) with very well defined ownership rules. Things are a bit more straightforward compared to any c++ project I believe. I just checked again, we don’t have any use-after free bugs in the bug history, probably that is because of %100 branch coverage test suite + fuzzing + sanitizers. So I rather adding another test to the suite than doing defensive programming. It is a personal choice I guess.
    - btrask 6y ago
      Every problem can be solved in many different ways. If you think you've already got use-after-free bugs under control, then more power to you! You absolutely have to concentrate your effort on whatever your biggest problems are. But I'll also say that if you don't have any use-after-free bugs in the history of a large C codebase... you might not even be on the lookout for them? I still have them sometimes, mainly when it comes to multiple ownership. And those are just the ones I found eventually. So yes, different strokes for different folks, but if you make the effort to incorporate tricks like this into your "unconscious" coding style, the ongoing effort is pretty minimal. Even if you decide this trick isn't worth it, there are countless others that you might find worthwhile. I'm always on the lookout for better ways of doing things.
nn3 6y ago
Better then #define ZFREE(p) do { free(p); p = NULL; } while(0)
- okl 6y ago
  You can even do #define free(p) do { free(p); p = NULL; } while(0) And when you want to call the original free: (free)(p); The preprocessor will not substitute this occurence of free.
  - devenblake 6y ago
    C n00b here - why the `do {} while(0)`? Couldn't you just do something like `#define free(p) { free(p); p = NULL; }`?
    - deaddodo 6y ago
      Semi-experienced C user here, I believe the anonymous block is perfectly adequate here. No idea why they are wrapping it in a single instance do loop, unless they’re unaware of block scoping or I’m unaware of some UB here.
      - btrask 6y ago
        do {} while(0) is a common idiom for macros in C, because it consumes the trailing semicolon, which a bare {} block doesn't do. if(x) MACRO(); else something(); expands to if(x) { ... }; // Error! else something();
- btrask 6y ago
  Here's the actual macro I (sometimes) use: #define FREE(ptrptr) do { \ __typeof__(ptrptr) const __x = (ptrptr); \ free(*__x); *__x = NULL; \ } while(0) There might be a better way of doing it though. Also, __typeof__() obviously isn't standard C. Edit to add: I've honestly been moving away from using a macro and just putting both statements on one line like in the OP. For something so simple, using a macro seems like overkill.

by btrask 6y ago

saagarjha 6y ago

Your approach requires extra checks, though, which are easy to forget. Also, NULL is not guaranteed to be the stored as zeros, plus padding is going to make your life annoying.

btrask 6y ago
Well, dangling pointers are also easy to forget... Yes, it requires some discipline. Good code requires discipline, doesn't it? The trick of checking that buffers are zeroed is purely a debugging tool, so it's okay if it doesn't work on some platforms. And if you allocate with calloc(), the padding will be zeroed for you. It's actually very rare that you will have to call memset() with this technique.
- saagarjha 6y ago
  > Good code requires discipline, doesn't it? This is like the most clichéd way of saying “my code has security vulnerabilities” that there is. I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things. > The trick of checking that buffers are zeroed is purely a debugging tool, so it's okay if it doesn't work on some platforms. Fair. > And if you allocate with calloc(), the padding will be zeroed for you. It might get unzeroed if you work with the memory.
  - btrask 6y ago
    All code is full of vulnerabilites. If you say your code isn't, then I'm sure it is. I just do the best I can to keep the error rate as low as possible. But it's a rate, and it's never zero. Also, it's not just about vulns in security-critical code. It's also about ordinary bugs. Why not be a little more careful? It won't hurt. > It might get unzeroed if you work with the memory. Maybe, but it isn't very common. I'm not sure when the C standard allows changing padding bytes, but in practice the compilers I've used don't seem to do it. And again, it's just a debugging aid, if it causes too much trouble on some platform, just turn it off.
    - saagarjha 6y ago
      It’s better to have automatic checks than rely on programmers being careful enough to remember to add them. For padding: this probably happens more on architectures that don’t do unaligned accesses very well.
      - btrask 6y ago
        Help me out here, because I'm really trying to understand. Are you saying that dangling pointers that blow up if you double-free them is an "automatic check"? If not, what kind of automatic check are you talking about? If the extra code is really that bothersome, just use a macro or wrapper function.
        saagarjha 6y ago
        It's a much better situation than NULLing them out, because that hides bugs and makes tools like Address Sanitizer useless. A dangling pointer, when freed, will often throw an assert in your allocator; here's an example of how this looks like on my computer: $ clang -x c - #include <stdlib.h> int main(int argc, char **argv) { char *foo = malloc(10); free(foo); free(foo); } $ ./a.out a.out(14391,0x1024dfd40) malloc: *** error for object 0x11fe06a30: pointer being freed was not allocated a.out(14391,0x1024dfd40) malloc: *** set a breakpoint in malloc_error_break to debug Abort trap As you turn up your (automatic) checking this will be caught more and more often. Setting the pointer to NULL will silently hide the error as free(NULL) is a no-op and nothing will catch it. Thus, the suggestion here was 1. advocating adding additional code, which has historically been hard to actually do in practice, and 2. providing a suggestion that is generally worse.
nsajko 6y ago
> NULL is not guaranteed to be the stored as zeros Is that a real issue, though? > padding is going to make your life annoying Just memset?
- huhtenberg 6y ago
  >> NULL is not guaranteed to be the stored as zeros > Is that a real issue, though? Of course, it's not, but that's one of those factoids that everyone learns at some point and feels like needing to rub it into everyone else's face assuming that these poor schmucks are as oblivious to it as they once were. A circle of life and all that.
  - saagarjha 6y ago
    Forgive me for encouraging the adoption of portable, compliant code to those who may not otherwise be aware of it. If you want to assume all the world’s an x86 that’s great but you should at least know what part of your code is going to be wrong elsewhere.
    - huhtenberg 6y ago
      Name a single platform where NULL is not 0.
- tjalfi 6y ago
  AMD GPUs use a non-zero NULL pointer[0]. [0] https://reviews.llvm.org/D26196 https://reviews.llvm.org/D26196
  - nsajko 6y ago
    Interesting. And it isn't even always non-zero; sometimes it's 32-bit -1, sometimes it's 32-bit 0 and sometimes it's 64-bit 0: https://llvm.org/docs/AMDGPUUsage.html#address-spaces https://llvm.org/docs/AMDGPUUsage.html#address-spaces
deleted 6y ago
[deleted]
spc476 6y ago
NULL is required to be stored as all zeros on POSIX systems.

annilt 6y ago

Please don’t get me wrong but these precautions sound like you are sweeping problems under the carpet which will come out one day back again. It sounds like you have ownership issues in the design and trying to hide ‘possible future bugs’. Do you use sanitizers for use-after free bugs? I see many people still don’t use them even though sanitizers have become very good in the last 5-6 years

btrask 6y ago
It's defensive coding. Do you think defensive driving is 'sweeping problems under the carpet'? (It is, but it's still useful...) I use every tool at my disposal. Sanitizers, static analyzers... and also not leaving dangling pointers in the first place. Why would I do anything less? It doesn't cost anything except a little effort. Take a look at this recent HN link: https://www.radsix.com/dashboard1/ https://www.radsix.com/dashboard1/ . Look at all those use-after-free bugs. Even if it only happens 1% or 0.01% of the time... It's a huge class of bugs in C code. Why not take such a simple step?
- annilt 6y ago
  If it works for you, then it is okay. It is not ‘a little effort’ for me to worry about someone else might use this pointer mistakenly, so I need to think about that all the time. It shifts my focus from problem solving to preventing future undefined behavior bugs. These bugs in the link, I don’t know C++, it is a big language which does a lot of things automatically, so it is already scary for me :) Maybe that is it, I write C server side code mostly(database) with very well defined ownership rules. Things are a bit more straightforward compared to any c++ project I believe. I just checked again, we don’t have any use-after free bugs in the bug history, probably that is because of %100 branch coverage test suite + fuzzing + sanitizers. So I rather adding another test to the suite than doing defensive programming. It is a personal choice I guess.
  - btrask 6y ago
    Every problem can be solved in many different ways. If you think you've already got use-after-free bugs under control, then more power to you! You absolutely have to concentrate your effort on whatever your biggest problems are. But I'll also say that if you don't have any use-after-free bugs in the history of a large C codebase... you might not even be on the lookout for them? I still have them sometimes, mainly when it comes to multiple ownership. And those are just the ones I found eventually. So yes, different strokes for different folks, but if you make the effort to incorporate tricks like this into your "unconscious" coding style, the ongoing effort is pretty minimal. Even if you decide this trick isn't worth it, there are countless others that you might find worthwhile. I'm always on the lookout for better ways of doing things.

nn3 6y ago

Better then #define ZFREE(p) do { free(p); p = NULL; } while(0)

okl 6y ago
You can even do #define free(p) do { free(p); p = NULL; } while(0) And when you want to call the original free: (free)(p); The preprocessor will not substitute this occurence of free.
- devenblake 6y ago
  C n00b here - why the `do {} while(0)`? Couldn't you just do something like `#define free(p) { free(p); p = NULL; }`?
  - deaddodo 6y ago
    Semi-experienced C user here, I believe the anonymous block is perfectly adequate here. No idea why they are wrapping it in a single instance do loop, unless they’re unaware of block scoping or I’m unaware of some UB here.
    - btrask 6y ago
      do {} while(0) is a common idiom for macros in C, because it consumes the trailing semicolon, which a bare {} block doesn't do. if(x) MACRO(); else something(); expands to if(x) { ... }; // Error! else something();
btrask 6y ago
Here's the actual macro I (sometimes) use: #define FREE(ptrptr) do { \ __typeof__(ptrptr) const __x = (ptrptr); \ free(*__x); *__x = NULL; \ } while(0) There might be a better way of doing it though. Also, __typeof__() obviously isn't standard C. Edit to add: I've honestly been moving away from using a macro and just putting both statements on one line like in the OP. For something so simple, using a macro seems like overkill.