I'm the author of The Silver Searcher (aka ag)[1]. I wrote my first C program almost 20 years ago, and I have some not-very-nice things to say about this guide. I hope the author won't take it personally.
First, Zed Shaw's Learn C the Hard Way is not worth reading. I agree with most of Tim Hentenaar's criticisms.[2] It's confusing, incomplete, and overly opinionated. If you want to get started with C, read the K&R book. It's not perfect, but it's better than anything else I know of.
As for the guide itself, the quality of its advice varies greatly. Some bits are great. Some are good. Some are controversial. Some are dangerously double-edged. And some are Not Even Wrong. It would take too long to go into detail, so I'll just say: Please don't blindly follow this guide. Explore other codebases (including the ones the author linked to). Talk to other programmers. Write your own projects, and get feedback from those who are more knowledgable.
You might notice that this advice generalizes to every language. That's because C isn't special. If anything, the language itself is simpler than most. C has just had more time to accumulate cruft, both technical and cultural. So don't feel intimidated. Once you learn those bits of historical trivia, you'll be fine.
Thanks for for the comment. I'm the author of the article. I'd really appreciate some more specific feedback, particularly the parts that you think are dangerous or "not even wrong."
I'll take a look at the Silver Searcher's code and see what pearls of wisdom I can gain.
In hindsight, my comment seems rather curt and dismissive. Thanks for taking my criticisms like a champ.
Honestly, I don't think ag is very good code. It's worth reading if you want to see optimizations like memory-mapped I/O, pthreads, Boyer-Moore strstr, etc. But if I had to grade the quality of the code, I'd give it a C-. Many parts have grown organically, and I haven't had time to restructure them. Also, ag's use-case is not exactly mission-critical. Nobody is going to die if it crashes or errors-out. Add my time constraints and… the result is some shoddy code.
I'll add "C Programming Guidelines" to my queue of blog posts to write, but I make no guarantees about when I'll get to it. Sorry.
This seems like a pretty good list. Focus on error handling is spot-on, most good C is just simply that.
One of my favorite tricks is to make your success case and your error cases look largely the same.
Say you have N allocations and N things that can fail. Lots of people get into trouble when they do:
void *p, *q, *r, *s;
p = a();
if (!p) { return ERROR; }
q = b();
if (!q) { free(p); return ERROR; }
r = c();
if (!r) { free(p); free(q); return ERROR; }
// etc..
And this madness just goes on and on. Add a new allocation? You potentially have to change n cleanup blocks.
I breathe a lot easier when I can do [note we abuse the fact that free(NULL) is a no-op]:
void *p = NULL;
void *q = NULL;
void *r = NULL;
p = a();
if (!p) goto cleanup;
q = b();
if (!q) goto cleanup;
r = c();
if (!r) goto cleanup;
cleanup:
free(p);
free(q);
free(r);
What if the buffer at "r" needs to return something to the caller? It turns out it's really easy to transfer ownership in this pattern:
// Success case. Give `r` to the caller.
*callerPointer = r;
r = NULL;
// Then we fall through to the failure block...
// r is null now so free(r) is harmless.
cleanup:
free(p);
free(q);
free(r);
This is The Way To Do It. I take minor issue to calling free(NULL) abuse, though (I know you get it, but allow me to soapbox). I've known programmers who get squeamish relying on this, as if it's a hack, even though it's been mandated by the standard forever. It's like they believe someone would put together a fully-functional libc and miss that simple requirement somehow.
I think the C standard library would be easier to work with if they would've taken this further, and required, for example, fclose(NULL) to be a no-op.
I think the C standard library would be easier to work with if they would've taken this further, and required, for example, fclose(NULL) to be a no-op.
There's always the counterargument that cleanup code that hides a missing allocation can mask bugs elsewhere, if for example a FILE * debug was never fopen()ed, only gets written to in exceptional cases, and fclose(NULL) ignores.
I'd personally err on the side of clearer clean-up code, but this a good point (and a view shared with glibc and BSD-derived libcs that intentionally segfault in this case).
I wouldn't describe that as an "intentional" segfault, but rather a "natural" one, the default behaviour you get on a system with an MMU. The null case with free() is a special-case extra check explicitly inserted in the code.
"Intentional" was perhaps the wrong word for glibc, but BSD libc (at least on FreeBSD and OS X) actually document it as intentional:
NOTES
The fclose() function does not handle NULL arguments; they will result in
a segmentation violation. This is intentional - it makes it easier to make
sure programs written under FreeBSD are bug free. This behaviour is an
implementation detail, and programs should not rely upon it.
I thought I remembered similar wording for glibc, but I appear to have been mistaken.
If you want to get nitpicky, you could also say there is performance cost to jumping into libc.so for free, then having it do a null check for you, then jump back to your code. Maybe that's worse than doing the null check more "locally".
But I do make a point whenever I'm writing C, if I write a cleanup function for a new type of struct or whatever, cleaning up null is always a no-op. It just makes life slightly less tedious. Having to remember which functions do this and which don't, I'll admit, is annoying.
I would venture that, if a few (essentially no-op) .so function calls are likely to affect performance to a point that you care, allocating and de-allocating heap memory under those same conditions might be a bit more of an issue.
Depends on the detail of the use case of course, but assuming that NULL cleanup is more the exception than the rule this is definitely a case of profile it first for me.
checks across your codebase will have a higher I$ footprint than sharing a single check inside free(), so the overall performance impact could easily be positive.
The code example was meant to illustrate the pattern while still being something that's semi-reasonable to write in a web form. Obviously in real life you'd want to do something with p, q, and r, which could make the chain of ANDs [not a pattern I am averse to in general] a bit more clumsy. YMMV.
I guess the other point I am trying to make is that success and failure can hit mostly the same code. Your snippet only checks for failure.
Not that error handling is unimportant, but my opinion is that C is about focusing on resource usage, and keeping very close track over giving back any resource you take, including memory, etc. If you do that, then the number of hard-to-debug runtime bugs drops significantly.
Yes, really. As in "goto" your high level languages and stop bothering me about what anyone working in a large enough code base will tell you is the correct way to do error handling in C.
Hint: grep linux sources for "goto" and re-evaluate your life decisions. :P
"Put one statement conditionals on same line as condition: if (rc < 0) goto fail;"
This is bad advice. A source-level debugger only works in source line numbers; if you ever need to set a breakpoint to find out when an error condition occurs you need to be able to set it on the action statement "goto fail" and not be bothered by the breakpoint tripping every time the if() gets evaluated.
Always separate the conditional from the statement.
I totally agre and that immediately popped out at me. When I started coding I would do this to 'save space', until burned by the breakpoint as you point out. Then I started putting it on the next line, indented, until a more seasoned developer told me about the gotchas of not having braces around a 'one-liner'. So now what used to take one line takes 4 lines, but I'm sure it will circumvent a 'goto fail' bug at some point!
Well, when I was learning c, Egyptian brackets apparently weren't even a thing. I have co-opted their use in certain places - I think it's a nice way to easily identify a lambda function, but I don't think I would ever use it in pure c. I simply like the way braces line up even if it takes up an extra line .. Kinda like reading HTML, you'd never put an opening tag at the end of a line, right? Anyway, I'm sure this topic has been exhausted a long time ago in a different post.
Hey Howard, thanks for the feedback. That's a good point that I hadn't really run into (probably depends on personal debugging style). I still think keeping error handlers small is worth it, but I will mention that tradeoff.
Maybe your source-level debugger only works in line numbers. Visual Studio doesn't. It knows that "goto fail" is a separate statement and you can place a breakpoint directly on that, not on the if statement, even though they're on the same line.
It won't make a difference for strings. But in C you can use a char to do math, when it will make a difference.
In fact, when working in constrained memory environments, like embedded 8 bit applications a char will often be used to do math, and then it makes a big difference. This is because there is no byte type by default in C.
CHAR_BIT must be at least 8 bits, but is not a "byte" as used by the original comment. The C standard defines "byte" differently from current common usage. To get exactly 8 bits of the desired signedness, which may not be possible on systems with CHAR_BIT greater than 8, one must use [u]int8_t.
----
From a draft version of the C standard:
The typedef name intN_t designates a signed integer type with width N , no padding
bits, and a two’s complement representation. Thus, int8_t denotes such a signed
integer type with a width of exactly 8 bits.
The typedef name uintN_t designates an unsigned integer type with width N and no
padding bits. Thus, uint24_t denotes such an unsigned integer type with a width of
exactly 24 bits.
These types are optional. However, if an implementation provides integer types with
widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a
two’s complement representation, it shall define the corresponding typedef names.
int8_t and uint8_t are definitely not possible on systems with CHAR_BIT greater than 8.
(From 6.2.6.1: Values stored in non-bit-field objects of any other object type consist of n × CHAR_BIT bits, where n is the size of an object of that type, in bytes.)
I believe unsigned char is the byte type, as you're allowed to convert each type into a unsigned char array and read it out (by some standard, was it C99 or C11, I don't know).
You're right, and that statement was poorly worded. If you check my project, I use plain char, which is signed on most platforms. The benefit of using unsigned char for binary data is the same, but the compiler warnings won't help on certain platforms.
The security brag list at the bottom linking qmail and seL4 is a little funny.
qmail has the property that (if it is indeed secure, which is a disputed claim) it's secure by virtue of djb's superhuman awesomeness. I'm happy to believe that djb can come up with secure elliptic curves, but I'm under no delusions that reading all his papers and coming up with my own elliptic curves is in any way a good idea. Similarly, even if I take the djb way of software writing to heart, I don't see a reason to believe that I could write software with qmail's level of robustness. I am just not a world-class expert the way that he is.
seL4, meanwhile, has a formal model of the C programming language in the proof assistant Isabelle/HOL, a formal model of what they want their C codebase to do (and not do), and a formal proof that, in fact, their C codebase has the properties they hope that it has. The C is handwritten, but it may as well not be; you can't do feature additions, significant refactoring, etc. to the C without needing to replicate those all in a language that is completely unlike C.
If these are the best arguments in favor of writing secure software in C, you don't really need any counterarguments....
If you read the history of C, written by Ritchie, he says that Johnson created it, because it was already clear in 1979 that not everyone was able to write correct C code.
And here we are with millions of dollars/euros/yans/... spent in all forms of analyzers, theorem provers, hardware extensions (MPX, cheri) to workaround language flaws.
Older C isn't better when it comes to security and avoiding bugs
K&R C is much beloved, but that innocent era is gone
We can't afford to fight each other, or C will really die
--
Older doesn't mean there wasn't error handling and comprehensive bug checking. Some of us have been writing defensive code for years. Age of "style" does not imply approach to security. OpenBSD has a great deal of C code and is security conscious.
Really, articles like these are surpurfulus to the fact that there are shitty coders out there in any language.
It's confusing if you're mallocing in one scope and freeing it in another one (foo is especially confusing, besides being a disaster ready to happen). But if you write bar() like this:
Unless I'm reading your example incorrectly, it's pretty easy to catch those if, in foo_destroy, you ensure that you don't try to free a null pointer.
> It's just a minimal example.
I know -- but it tends to be confusing if you free() something in another scope than the one where you're malloc()-ing it. That's why your example, even if minimal, seemed confusing.
The obvious exception is if you have some reference counting mechanism/GC mechanism in place -- but then the wise thing to do is to not manually touch pointers all over the code.
If `foo` doesn't own `buf`, it should be declared `char * const` (or `char const * const`). If `foo` takes owner ship of `buf` (and is intended to free it), it should take a pointer to the pointer to clear it for the caller. This is one way to write it:
You can dereference `bufptr` at the top of the function to make the code more readable. This is how (almost) every object freeing function works in my project.
Both can easily be misunderstood when you're quickly scanning through a bunch of code. Screen space isn't at such a premium that you can't afford a couple extra lines in a conditional statement.
Why would you indent hat next line? Presumably because you were trying to add it to the existing if statement's non-existant block. But why would you do that if you don't see a block?
The problem with
if (rc < 0)
return rc;
is that we see the indentation and infer that there must be a block, so it's tempting to try to add to the block.
When you keep your entire conditional statement on one line, it looks, to someone scanning the code, like just any other line. It doesn't look like a block at all, in fact it looks more like a function call.
> Why would you indent hat next line?
There are lots of reasons someone might indent the next line. Maybe they were in a rush. Maybe they were distracted by something else while they were working. Maybe they were tired. Maybe they were drunk.
Mistakes will happen. Ideally wrong code will look wrong. All of the following examples do the same thing. If you know the code is supposed to call Foo() and Bar() if bVar is true, which of these examples make it obvious that something is wrong?
if (bVar) Foo();
Bar();
if (bVar)
Foo();
Bar();
if (bVar)
{
Foo();
}
Bar();
if (bVar)
{
Foo();
}
Bar();
The third, followed by the fourth, makes the mistake most obvious to me.
The one thing I've never seen mentioned in style guide is that maybe the same piece of code should take the style of whatever is fitting in its context?
For example, let's say there are 20 case where it's a single line return, then shouldn't be "if(rc < 0) return rc;" better. On the other hand, if the other condition in that block of code all need brackets, it just seems good sense for the single line condition to have bracket too.
It could be extending pretty much to all other syntactic sugar even in higher level language (map + lambda vs for loop?)
This may be a controversial claim, but in programming often style is a major part of substance. Even in the event your program is correct if it's stylistic written in such a way that it makes it easier to break in future edits then it's a lower quality program then it could have been if written with better style. In my view the quality of a developer is to a large degree expressed by the style of their code.
(Edit: This is not to say that there can't be multiple good stylistic options, just more that the bad ones by far outnumber the good ones and at the end of the day it does matter how your code looks if you plan on supporting it for a long time)
"Use struct thing thingptr[1]; to get pointers with inline storage
...
You should pretty much always use -> instead of .
...
I think this is the most important trick for making C code beautiful"
I don't really get what they're after with this section. Can anyone shed some light?
If I saw a declaration like that in a code I was reviewing I would politely ask not to use this pointless level of indirection. This adds a tiny bit of complexity to the code and gives nothing back.
If all it takes is one human screw up with 3 lines of code to get full remote execution, you will inevitably get security bugs with that code. A best practices guide will not work, because people will not execute on their best practices %100 of the time. A language specification or at least a linter will enforce it %100 of the time in a practical comparison. This is why people want to sunset C family languages.
I do mostly embedded programming, which is overwhelmingly dominated by C. The reason I think C will probably never be replaced as the dominant low level language is inertia. Every microprocessor manufacturer knows they have to provide drivers, libraries, and board support packages for their parts. If they don't, no one will use those processors, because of the extra effort involved, and the fact that other companies do provide them. All of these drivers, libraries, and board support packages are written in C. Writing them in something other than C is basically equivalent to not writing them at all, because instead of forcing developers to reinvent the wheel, you're forcing them to use a language most of them don't know. In the end, it's the exact same problem: a large amount of effort required to get to the point where you can start implementing your application.
This same logic applies to real time operating system vendors. Everything is written in C. The minute they start writing their code in a different language is the minute their competitors become that much more attractive. On top of that, one of the major selling points of the major RTOSes is that they have mature code bases with decades of bug fixes and products using them.
Perhaps this presents an opportunity for disruption, but it seems like a steep hill to climb.
sqlite and lmdb aren't exactly equivalent, but I guess that is his point; you can gain a lot of performance just by understanding your use case and what tradeoffs you can make.
If you're dealing with POSIX / the C stdlib, mirror it and use char* . (The string version of "Respect the integer types used by each API."; if you're coding for Windows you'll likely need to use LPWSTR or whatever they call it now.) But: if you're storing UTF-8 string data, you _want_ unsigned char (and maybe uint8_t[2]): if start trying to inspect the code units, esp. to decode them to code points, you're going to need to do comparisons and bit manipulations on them, and its too easy to write `code_unit < 0x80` or something similarly silly (e.g., while masking bits to see if it's a continuation byte). It's like the article early today called "Should I use Signed or Unsigned"[1] states: "Prefer unsigned when using bitwise operators".
> Limit recursion
Recursion is fine; you simply need to be aware of your worst case stack depth. You're looking, really, for the complexity of your stack usage, or the big-oh of your stack size. If you're doing something like recursing down an in-memory balanced binary tree, it's O(log(n)) in the worst case: ~64 stack frames on a 64-bit CPU. Can you fit that many on the stack? Certainly that's heavier than a normal function call, but it's still a finite, computable size.
If you can't compute that / prove an upper bound, then I agree.
> Aside: any language that uses keywords like public and private to determine visibility instead of placement/scoping is bad
If not with keywords, how should visibility be determined? (How does "placement/scoping" determine visibility?)
> Use get/init or create/copy/alloc in function names to indicate ownership versus borrowing for return values
Inevitable, I feel like you end up creating some structure, let's call it a Foo. So you then have a Foo * Foo_create(), a Foo_destroy(Foo * ). Some action performable on Foo? Foo_reticulate_splines(Foo* , …). Combined with,
> You should generally track used and unused space in dynamic arrays,
and the overhead of writing functionality to deal with dynamic arrays in C, the entire section about managing strings, and you've come to the very reason I left C: I'm already writing C++. Foo_create is a ctor; Foo_destroy a dtor; Foo_reticulate_splines a member function. Combined with vectors & strings taking care of day-to-day memory management, and I'm left with little reason to use C.[3] RAII adds some degree of object lifetime management that simply isn't present in C (but does not add the compile-time checks that I'm starting to love Rust for.)
Back when I got to learn it, around 1993, I had already used Turbo Pascal 3, 5.5 and 6.0.
C seemed so primitive and unsafe in regards to Turbo Pascal features.
However the same person that got me into C, also showed me C++.
There it was, a language that could be used as safely as Turbo Pascal, provided one made use of the language features instead of the C copy-paste compatibility.
When Turbo Pascal stopped being a possibility, I joined ranks with the C++ side, in the C vs C++ flame wars.
Even teached C++ at the university as a senior student.
Only used C instead of C++, when asked to do so by university teachers or customers.
For memory allocation you often don't need to use malloc. Many (most?) programs can mmap a large region of memory and allocate from that. Then when you're done you just unmap the region. This is much easier to use since you only have to remember to unmap once as opposed to handling each allocated pointer individually.
Also, to clarify, I'm not saying allocate yourself a big piece of memory and then reimplement malloc on top of that. You literally just get a big piece of memory and every time you want to allocate something new from it add the offset to the base address, return that to the user, and store the new offset.
1) Reinventing malloc for no good reason
2) Opening yourself up to a whole world of security holes
See Bob Beck's talk on the first 30 days of LibreSSL. Heartbleed wouldn't have been nearly as catastrophic if OpenSSL just used the system memory allocator like normal people.
There are a few good reasons to reimplement malloc(), but of course it needs to be done very very carefully. For example, with a naive buddy list allocator it is (or was, in 2007) possible to more than triple allocation throughput vs. malloc(), but one loses many of malloc()'s security features.
I don't understand how this could lead to a security hole any more easily than using malloc could. Can you give an example? The system memory allocator is using mmap internally anyways (on Linux and anywhere else using the Doug Lea allocator at least) and a simple region based allocator is only a couple dozen lines of code.
Edit: also, you're not reinventing malloc for no good reason. A region based allocator is much faster, possibly at the expense of using more space than the system allocator (because you might reserve more space than you actually need).
The point is that when you use malloc, an attacker exploiting a buffer overrun can't easily guess at the offset they need to find useful data. When you allocate everything in a big region, your process can be expected to read past the object bounds, so an attacker might be able to probe the address space (also the memory layout might be more deterministic). If they try to probe like that in a program that allocates with malloc, they'll just segfault the program.
If you want to count examples you're reasoning about this the wrong way. The question is what way leads to a higher probability of making a mistake, not the existence of discrete examples. Considering that one way, Valgrind works out of the box, but with your way, it doesn't, I think the answer to that question is quite clear. Even if there weren't tools like that, it is easier to read code and understand pointers' lifetimes when they're being handled individually, instead of having their lifetimes be part of some far-off region.
The whole point of using regions is not to be worrying about pointer lifetimes. And you don't need Valgrind when you have regions since you aren't going to leak memory by forgetting to free something. Valgrind solves a problem that doesn't exist when you use regions.
If you're really worried that you'll leak an entire region worth of data just allocate the region with malloc instead of mmap and then use Valgrind to tell you what regions you aren't destroying.
The risk is that you'll screw up and write a buggy allocator with a security hole.
Yes, if your program just needs to allocate lots of memory, do some computations, then exit, this approach works. But the programs where security is most critical do not usually follow that pattern.
The code for this is so simple it would be hard to screw up. Plus you can just use obstacks[1] which pretty much provide the interface I was describing.
Look, I'm totally willing to accept that there might be flaws with this approach, but with the exception of adrusi, no one's objections have been all that reasonable. If there are legitimate objections (not just handwavy, oh, you'll probably implement the allocator wrong) I'd love to hear them. I use this pattern in my own code and I'll stop if there are legitimate flaws.
Also, Akamai released a patch for the OpenSSL allocator bug mentioned earlier. Guess what the patch used: mmaped regions.[2]
Would you happen to have some code online I can look at which use this technique ?
I first saw Casey Muratori from https://handmadehero.org/ use an approach like this and used the same approach in a small project of mine. It worked well and was simple enough to implement.
Because you're going to have bugs. You don't need the extra performance, and you do need the extra safety. There is no good reason for you to do this in 2015, unless your system's malloc is broken. The exceptions to this rule know who they are.
Yes, safety was the initial motivation. The advantage is that you free once and the semantics are more stack like. The performance is an additional benefit.
It is entirely possible to have no bugs in the <100 lines needed for this code, so I don't think the bug argument is valid. adrusi did raise a legitimate concern however.
Setting aside the wisdom of this approach, if you were going to do this why wouldn't you just use malloc() to allocate the single large arena that you're going to allocate from yourself? It's still just one free().
In other words, the malloc-versus-mmap distinction here isn't the important one, it's the custom allocator versus system allocator one.
That would work too. malloc() uses mmap() internally for large allocations, so the reason to use mmap() is just to skip all the other stuff malloc() does (which you don't care about if you have a custom allocator).
For memory allocation you often don't need to use malloc.
True.
Many (most?) programs can mmap a large region of memory and allocate from that.
You don't even need to do that for many applications. Coming from an embedded systems background where dynamic allocation is highly discouraged and almost never necessary, the amount of superfluous allocation/deallocation I see in a lot of code is astounding; maybe it's because of my experience, but I can often very easily find a way to do something without dynamic allocation and consequently O(n) space, with what usually turns out to be simpler (thus less chance of being buggy) code and O(1) space.
But when you do need dynamic allocation, malloc() isn't bad at all.
Hey Jeff, I had to double check because I couldn't believe this was you. I'm a big fan of the Jeff and Casey Show!
The problem with doing custom allocation is not specifically due to bugs in the allocator per se, but because your custom allocator will probably not be hardened against other bugs like some other allocators are. Specifically you will piss off the OpenBSD people who have put a lot of work into their allocator with "exploit mitigations."
A bump allocator is pretty much guaranteed to let overflows (reads and writes) touch other valid memory. A hardened allocator will use guard pages and other tricks to prevent those bugs from being exploitable.
One thing you can do is use bump allocators within "security zones." For example data related to a single user can be allocated together without much risk, but data from separate users is kept separate.
This is very different from the world of game programming. I've been wondering lately if there will be some sort of bifurcation of the field.
Ahhh, this makes a lot of sense. Also, I think you're mixing me up with Jeff Roberts. I'm Jeff Rogers :). But yes, the Jeff and Casey Show is awesome :).
First, Zed Shaw's Learn C the Hard Way is not worth reading. I agree with most of Tim Hentenaar's criticisms.[2] It's confusing, incomplete, and overly opinionated. If you want to get started with C, read the K&R book. It's not perfect, but it's better than anything else I know of.
As for the guide itself, the quality of its advice varies greatly. Some bits are great. Some are good. Some are controversial. Some are dangerously double-edged. And some are Not Even Wrong. It would take too long to go into detail, so I'll just say: Please don't blindly follow this guide. Explore other codebases (including the ones the author linked to). Talk to other programmers. Write your own projects, and get feedback from those who are more knowledgable.
You might notice that this advice generalizes to every language. That's because C isn't special. If anything, the language itself is simpler than most. C has just had more time to accumulate cruft, both technical and cultural. So don't feel intimidated. Once you learn those bits of historical trivia, you'll be fine.
1. https://github.com/ggreer/the_silver_searcher
2. http://hentenaar.com/dont-learn-c-the-wrong-way