4 ms·

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 buff

by 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.
        btrask 6y ago
        Good points, thank you for explaining! I can see an argument for wrapping it in a macro so you can turn off nulling in debug builds (ASan might even have hooks so you can automate this, I know Valgrind does). But use-after-free is worse than just double-frees, and if you read a dangling pointer in production there's no real way to catch it AFAIK. Last I heard (admittedly been a few years since I checked), you're not supposed to deploy ASan builds because they actually increase the attack surface. So, your program's memory is full of these dangling pointers, and at some point you will have a bug you didn't catch and use one. And you can't even write an assertion to check that it's valid. What do you propose? And again to clarify, I'm not trying to advocate for hiding bugs. I want to catch them early (e.g. with assertions), but I also want to avoid reading garbage at runtime at all costs, because that's how programs get pwn'd.
- okl 6y ago
  > I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things. That's not what the parent comment said.
  - saagarjha 6y ago
    I’m not sure what it could have said after saying that programmers should have disciple after I mentioned that their thing required extra checks to work.
    - okl 6y ago
      Parent said: "Good code requires discipline, doesn't it?" You retort: "I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things." I think that is a dishonest misrepresentation of what the parent comment said, isn't it?
      - saagarjha 6y ago
        The “discipline” in this case (see the whole thread) is “have programmers remember to insert checks”, which has historically been a good way to have security holes crop up. So I’m not sure what was dishonest about it?
        btrask 6y ago
        I don't think that's fair in this case because nulling out pointers isn't the first line of defense. If you forget to do it once, it's not going to cause a bug in and of itself. You can easily grep the code periodically to find any cases you missed.
        Quekid5 6y ago
        Indeed, it shouldn't be a first line of defense (nulling + an assert seems reasonable, fwiw), and accessing a nulled out pointer is just as UB as any other UB. It's probably more likely to crash immediately in practice, but it's also easier for an optimizer to "see through", so you may get surprising optimizations if you get it wrong. Honestly, unless you really cannot afford it time-budget wise, I would just ship everything with ASAN, UBSAN, etc. and deal with the crash reports.

by btrask 6y ago

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.
      - btrask 6y ago
        Good points, thank you for explaining! I can see an argument for wrapping it in a macro so you can turn off nulling in debug builds (ASan might even have hooks so you can automate this, I know Valgrind does). But use-after-free is worse than just double-frees, and if you read a dangling pointer in production there's no real way to catch it AFAIK. Last I heard (admittedly been a few years since I checked), you're not supposed to deploy ASan builds because they actually increase the attack surface. So, your program's memory is full of these dangling pointers, and at some point you will have a bug you didn't catch and use one. And you can't even write an assertion to check that it's valid. What do you propose? And again to clarify, I'm not trying to advocate for hiding bugs. I want to catch them early (e.g. with assertions), but I also want to avoid reading garbage at runtime at all costs, because that's how programs get pwn'd.
okl 6y ago
> I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things. That's not what the parent comment said.
- saagarjha 6y ago
  I’m not sure what it could have said after saying that programmers should have disciple after I mentioned that their thing required extra checks to work.
  - okl 6y ago
    Parent said: "Good code requires discipline, doesn't it?" You retort: "I have yet to see code that has remained secure solely on the “discipline” of programmers remembering to check things." I think that is a dishonest misrepresentation of what the parent comment said, isn't it?
    - saagarjha 6y ago
      The “discipline” in this case (see the whole thread) is “have programmers remember to insert checks”, which has historically been a good way to have security holes crop up. So I’m not sure what was dishonest about it?
      - btrask 6y ago
        I don't think that's fair in this case because nulling out pointers isn't the first line of defense. If you forget to do it once, it's not going to cause a bug in and of itself. You can easily grep the code periodically to find any cases you missed.
        Quekid5 6y ago
        Indeed, it shouldn't be a first line of defense (nulling + an assert seems reasonable, fwiw), and accessing a nulled out pointer is just as UB as any other UB. It's probably more likely to crash immediately in practice, but it's also easier for an optimizer to "see through", so you may get surprising optimizations if you get it wrong. Honestly, unless you really cannot afford it time-budget wise, I would just ship everything with ASAN, UBSAN, etc. and deal with the crash reports.