27 ms·
Git's list of banned C functions
- maxk42 6y agoWhat would be helpful is an explanation of how each function ends up being misused so people can learn from this.
- petters 6y agoGit blame is helpful here. See e.g.https://github.com/git/git/commit/1b11b64b815db62f93a04242e4aed5687a448748 https://github.com/git/git/commit/1b11b64b815db62f93a04242e4...
- jsmith45 6y agoView the git history for the file. Each commit that adds functions has a detailed explanation of what is wrong with the functions.
- zbendefy 6y agoAre there some details on whats wrong with these?
- bvaldivielso 6y agoThe commit messages that added them explain the reasoning
- ufo 6y agoI wish they would have put that on comments instead of on the commit messages. It's not the first time that I've seen this particular list of banned functions being shared online and every time it happens someone has to explain that the most interesting info is hidden in the commit messages.
- alexchamberlain 6y agoAll the string functions have buffer overrun vulnerabilities if not used carefully. I'm not sure about the time functions though.
- edflsafoiewq 6y agoThe time functions are either non-reentrant, or, for the _r versions, have the same problem with buffer overruns. https://github.com/git/git/commit/1fbfdf556f2abc708183caca53ae4e2881b46ae2 https://github.com/git/git/commit/1fbfdf556f2abc708183caca53... https://github.com/git/git/commit/91aef030152d121f6b4bc3b933c696073ba073e2 https://github.com/git/git/commit/91aef030152d121f6b4bc3b933...
- trilinearnz 6y agoVery much this. I frequently write small games in C, and the number of times I have been bitten by baffling behaviour because a string somewhere was copied into an array that was too short, are many! Apart from that, I love the simplicity of the language and the stdlib, and it's definitely my preferred hobby programming environment. It would be good to know what the commonly-accepted alternatives are.
- deleted 6y ago[deleted]
- csours 6y agoI'm pretty sure you could google each of these with the word 'dangerous' For example: https://lgtm.com/rules/2154840805/ https://lgtm.com/rules/2154840805/
- bvaldivielso 6y agoAh this is a very good idea. I guess you still have to make sure that all your translation units include this header, which isn't completely foolproof. Static analysis would probably be more robust, but way more involved.
- radus 6y agoBest of both worlds: use static analysis to ensure the header is included?
- koenigdavidmj 6y agogcc has a -include option, so this can be done once in the Makefile and get the benefit everywhere (unless you’re being clever).
- Athos_vk 6y agoI remember visual studio having an option to force include a file, surely something like that would exist for other toolchains
- kccqzy 6y agoYou don't need fancy static analysis. You can find out whether the banned functions are called just by inspecting the compiled object file. Add it to the build step and done.
- drfuchs 6y agoIt would be nice if the error messages generated would suggest replacement functions that they deem appropriate. I see that I'm not supposed to use gmtime, localtime, ctime, ctime_r, asctime, and asctime_r; but what do they think I should use?
- colordrops 6y agoAlso, why the functions are banned.
- cle 6y agoFrom the commit messages > The ctime_r() and asctime_r() functions are reentrant, but have no check that the buffer we pass in is long enough (the manpage says it "should have room for at least 26 bytes"). Since this is such an easy-to-get-wrong interface, and since we have the much safer strftime() as well as its more convenient strbuf_addftime() wrapper, let's ban both of those. (https://github.com/git/git/commit/91aef030152d121f6b4bc3b933c696073ba073e2 https://github.com/git/git/commit/91aef030152d121f6b4bc3b933...) > The traditional gmtime(), localtime(), ctime(), and asctime() functions return pointers to shared storage. This means they're not thread-safe, and they also run the risk of somebody holding onto the result across multiple calls (where each call invalidates the previous result). All callers should be using their reentrant counterparts. (https://github.com/git/git/commit/1fbfdf556f2abc708183caca53ae4e2881b46ae2 https://github.com/git/git/commit/1fbfdf556f2abc708183caca53...)
- drfuchs 6y agoYes, but every hapless user shouldn't have to go searching through a bunch of commit messages to find the suggested replacement. Bad UX.
- capableweb 6y agoThe UX of using this list is not by manually searching through the list and seeing the reason behind them. You include the file together with the rest of your sources and now you get compilation errors if you try to use them. Can't think of a better UX for banned functions. Discovering why the thing is banned you only have to do once, if you care. If you're just modifying something quickly and minor in Git, you might not even care why.
- captainmuon 6y agoIt would be interesting to see the rationale behind these bans, and what the suggested alternatives are. Some are obvious, like `strcpy`, but I can't remember what the problem with `sprintf` or the time functions are. If you are doing something like `sprintf(buffer, "%f, %f", a, b)`, yes it is tricky to choose the size of buffer frugally, but if you replace that by `ftoa` and constructing the string by hand, you are likely to introduce more bugs. Edit: as pointed out in another post, you can do git blame to see the rationale for each ban, quite interesing.
- dahfizz 6y agoThis was my reaction as well. Banning strncpy just encourages haphazard manual copying.
- asdfasgasdgasdg 6y agoI think you're meant to use snprintf instead. It would be great to see documentation on the alternatives!
- smasher164 6y agoFrom the commit message: If you're thinking about using it, consider instead: - strlcpy() if you really just need a truncated but NUL-terminated string (we provide a compat version, so it's always available) - xsnprintf() if you're sure that what you're copying should fit - strbuf or xstrfmt() if you need to handle arbitrary-length heap-allocated strings
- nwmcsween 6y agostrlcpy is safer but effectively running strlen(src) every call is a good wtf
- azurezyq 6y agomaybe this https://github.com/git/git/blob/master/strbuf.h https://github.com/git/git/blob/master/strbuf.h ?
- 6y ago
- attractivechaos 6y agoI wonder how they copy strings with strcpy and strncpy both banned. strlcpy? But it is not conforming to major standards. Or just memcpy with extra code?
- dgentile 6y agoEdited: Looks like they have safe alternatives: " - strlcpy() if you really just need a truncated but NUL-terminated string (we provide a compat version, so it's always available) - xsnprintf() if you're sure that what you're copying should fit - strbuf or xstrfmt() if you need to handle arbitrary-length heap-allocated strings "
- lights0123 6y agohttps://github.com/git/git/commit/e488b7aba743d23b830d239dcc33d9ca0745a9ad https://github.com/git/git/commit/e488b7aba743d23b830d239dcc... Yes: > we provide a compat version, so it's always available
- deleted 6y ago[deleted]
- attractivechaos 6y agoThis gets me interested. Link [1] below shows their implementation of strlcpy(). This is a questionable implementation. With strncpy, the source string "src" may not be NULL terminated IIRC. The git implementation requires "src" to be NULL terminated. If not, an invalid read. EDIT: according to the strlcpy manpage [2], "src" is required to be NULL terminated, so strlcpy imposes more restrictions and is not a proper replacement of strncpy. Furthermore, imagine "src" has 1Mb characters but we only want to copy the first 3 chars. The git implementation would traverse the entire 1Mb to find the length first, but a proper implementation only needs to look at the first 3 chars. So, they banned strncpy and provided a worse solution to that. [1]: https://github.com/git/git/blob/master/compat/strlcpy.c https://github.com/git/git/blob/master/compat/strlcpy.c [2]: https://linux.die.net/man/3/strlcpy https://linux.die.net/man/3/strlcpy
- paultopia 6y agoIts really wild, as a person coming from other languages who has written maybe ten lines of C in his life that the functions that seem to be massive footguns in C are, like, "format a string" or "get time in GMT." That's... really scary.
- ironmagma 6y agoYeah, there is a culture of complacency in C probably owing to the enormous historical baggage of legacy code that has to be supported and the blurred line between stdlib and system call.
- Spivak 6y agoI mean on Linux you're not encumbered by this because the syscall api is stable but in practice most GNU/Linux distros assume glibc. You can't correctly resolve a hostname on Linux without farming out to glibc -- hell even the kernel punts to userspace for dns names but you can technically ignore it if you want. On BSDs and macOS you're always SOL because the syscall api isn't stable and only the C wrappers are.
- dangerbird2 6y agoc standard library doesn't really relate directly to system calls (at least in modern os'es). In particular, the stdio.h functions are buffered by default, while their system call analogues are not. For unixes, system call wrappers are typically found in <unistd.h>, not the "official" c standard library
- freedomben 6y agoI disagree completely. Devs who use C are the least complacent about security in my experience. The problems are from previous eras before they knew about many of these things. A ton of people in modern languages couldn't name a single dangerous function, though they do exist in every language. You'd be amazed at how many race condition vulns result from TOCTOU errors just in authentication, or checking for the existence of a file before opening it, etc. It's absolutely true that decades ago the C community was complacent, but it's not true now. Source: I taught secure coding in C/C++ in the 00s.
- shadowgovt 6y agoTo its credit, it's convenient that the C pre-processor is so powerful that it facilitates baking a "C the good parts" concept directly into the compilation process.
- moomin 6y agoThey should probably add sscanf.
- ed25519FUUU 6y agoFirst thing I looked for. It looks like it was used here: https://github.com/git/git/blob/master/object-file.c#L1293 https://github.com/git/git/blob/master/object-file.c#L1293 And currently used here (at least): https://github.com/git/git/blob/master/refs.c#L1235 https://github.com/git/git/blob/master/refs.c#L1235
- deleted 6y ago[deleted]
- EdSchouten 6y agoFunnily enough, strtok() is not listed :)
- sys_64738 6y agoscanf?
- sys_64738 6y agogetc?
- syncsynchalt 6y ago`gets` would be the ultimate banned C function, I suspect nobody thought it was worth spelling out though.
- ape4 6y agoJust replace strcpy(a,b) with strcpyn(a,b,INT_MAX) /joke
- fatnoah 6y agoI'm pretty sure I've seen similar logic in my life.
- beeforpork 6y agoBeen a while, eh? It should be strncpy(a,b,(size_t)-1)!
- guerrilla 6y agoSIZE_MAX does exist.
- snvsn 6y agoPrevious discussion: https://news.ycombinator.com/item?id=20792938 https://news.ycombinator.com/item?id=20792938
- StillBored 6y agoThese functions are one of the many reasons why I tend to have a C with some C++ classes dialect I use in my own projects. std::string needs some tweaks, but it can mostly be treated as a built in and it wipes out a huge set of C string issues.
- lmilcin 6y agoTo respond to some of the comments. It is not that there is anything intrinsically wrong with these functions. You can technically use all of them and I have been using all of them, safely, for decades. The issue is they are huge traps to the point that in a larger piece of software one can say "well, it's just not worth it". You can go much, much, much further than that. In couple embedded projects I worked some of the rules were: * dynamic allocation after application has started is banned -- any heap buffers and data structures must be allocated at the start of the application and after that any allocation is a compile time error, * any constructs that would prevent statically calculating stack usage were banned (for example any form of recursion except when exact recursion depth is ensured statically), * any locks were banned, * absolutely every data structure must have size ensured, in a simple way, beyond any reasonable doubt, etc.
- xondono 6y agoAnything enforcing MISRA has essentially (almost) no way of allocating memory at runtime.
- fsociety 6y agoIt’s funny, I worked exclusively with MISRA at the start of my career. Eventually I started a job at a FAANG and received quizzical comments on why I implemented a memory arena. The argument was to allocate memory freely and let it pool memory as necessary. Fair enough, it was simpler and fit the standard expectation of development. The issue is that if you talk with the allocator team they complain of not being able to fix performance issues fast enough due to allocations firing off left and right in the middle of a request. I never realized that my view of C programming is heavily influenced by MISRA until your comment. I know game engine programming follows a similar, perhaps unspoken, convention.
- munchbunny 6y agoThe lack of runtime allocations in game engine programming comes from a different motivation: allocations are expensive, garbage collections are expensive, cache coherency matters, and you're chucking around a lot of very similar looking objects, so... object pools!
- xvilka 6y agoI hope, one day to see it's rewritten in a safer language.
- qbasic_forever 6y agoThere's a nice Go implementation of git: https://github.com/go-git/go-git https://github.com/go-git/go-git
- amir734jj 6y agoMaybe instead of just writing a banned message, it should be the name of alternative function to use.
- rcgorton 6y agoBut it isn't even April 1 yet! This is truly a BAAAD joke. So GIT is not implemented in C? Or C++?
- jancsika 6y agoI love seeing "strncpy" right after "strcpy." If someone wants some fun, try this: 1. Slurp up all the FOSS projects that extend back to 90s or early 2000s. 2. Filter by starting at earliest snapshot and finding occurrences of strcpy and friends who don't have the "n" in the middle. 3. For those occurrences, see which ones were "fixed" by changing them to strncpy and friends in a later commit somewhere. 4. See if you can isolate that part of the code that has the strncpy/etc. and run gcc on it. Gcc-- for certain cases (string literals, I think)-- can report a warning if "n" has been set to a value that could cause an overflow. I'm going to speculate that there was a period where C programmers were furiously committing a large number of errors to their codebases because the "n" stands for "safety."
- gilbetron 6y agoMeh, most of us understood the sharp edges of strings pretty well. Before, we'd check the len of strings before strcpy, strncpy let us do it without doing that, and just slap a 0 in if needed. Safe? No. Better? A bit. Do I ever want to do string manipulation again with C? Nope.
- tomjakubowski 6y agoUnderstanding the sharp edges is one thing. Being able to avoid them in practice is another. The history of memory safety problems in C string handling, especially involving strcpy/strncpy, strongly suggests to me that they're unavoidable even for C programmers who are skilled, knowledgeable, and experienced.
- Tarragon 6y agoAs an embedded programmer mostly working with C who considers themselves skilled, knowledgeable, and experienced... I agree.
- commandlinefan 6y agoOk, memcpy(dst, src, strlen(src)) it is then!
- Luyt 6y agoIt would be great if the BANNED() macro could suggest the correct function to use.
- abetusk 6y agoThe Git Mailing List Archive on lore.kernel.org (found in the README from the git mirror on GitHub) has more context [0] [1] [2]. From Jeff King on 2018-07-24: The strncpy() function is less horrible than strcpy(), but is still pretty easy to misuse because of its funny termination semantics. Namely, that if it truncates it omits the NUL terminator, and you must remember to add it yourself. Even if you use it correctly, it's sometimes hard for a reader to verify this without hunting through the code. If you're thinking about using it, consider instead: - strlcpy() if you really just need a truncated but NUL-terminated string (we provide a compat version, so it's always available) - xsnprintf() if you're sure that what you're copying should fit - strbuf or xstrfmt() if you need to handle arbitrary-length heap-allocated strings I just did a search on the keywords 'banned' and 'strncpy' [2] [0] https://lore.kernel.org/git/20180724092828.GD3288@sigill.intra.peff.net/ https://lore.kernel.org/git/20180724092828.GD3288@sigill.int... [1] https://lore.kernel.org/git/20190103044941.GA20047@sigill.intra.peff.net/ https://lore.kernel.org/git/20190103044941.GA20047@sigill.in... [2] https://lore.kernel.org/git/20190102093846.6664-1-e@80x24.org/ https://lore.kernel.org/git/20190102093846.6664-1-e@80x24.or... [3] https://lore.kernel.org/git/?q=banned+strncpy https://lore.kernel.org/git/?q=banned+strncpy
- js2 6y agoPsst: https://github.com/git/git/commits/master/banned.h https://github.com/git/git/commits/master/banned.h (Git development is done by emailing patches. Those patches include the git commit message, which we can see just by looking at the history of the file. Sometimes there's additional discussion on the ML, but the most important details are in the commit message because the git development team is very disciplined about that.)
- abetusk 6y agoHa, yep, whoops
- TheRealSteel 6y agoI'm an idiot, I read the headline and thought these were banned from Git entirely. As in, you couldn't commit them to any repo using Git, at all. Thought that seemed a bit harsh. Turns out you just can't use them when you contribute code to the Git project. That makes sense, and seems reasonable.
- edgyquant 6y agoCritiquing poor code practices is beyond the scope of git at this time
- TheRealSteel 6y agoShould be easy to implement, will have a pull request ready tomorrow. Edit: wait, I can't use strcpy?! Screw that, then I'm not open sourcing my AGI!
- Animats 6y agoAbout 20 years too late. Those should have been moved to a "deprecated" header file decades ago.
- deleted 6y ago[deleted]
- kgrimes2 6y agoCan a C guru provide a TL;DR of why these are bad?
- syncsynchalt 6y ago- strcpy: no bounds check - strcat: no bounds check - strncpy: does not nul-terminate on overflow - strncat: no major issues, probably to force usage of strlcat - sprintf: no bounds check - vsprintf: no bounds check - gmtime: returns static memory - localtime: returns static memory - ctime: no bounds check - ctime_r: no bounds check - asctime: returns static memory - asctime_r: no bounds check The str functions all have safer alternatives. The time functions have reentrant alternatives, and/or alternatives that provide a bounds check.
- 1337_d00dZ 6y agoIn compilers that implement GCC extensions (such as Clang), you can use the "poison" directive to achieve the same effect (but with a better error message): #pragma GCC poison printf sprintf fprintf [0] https://gcc.gnu.org/onlinedocs/gcc-3.2/cpp/Pragmas.html https://gcc.gnu.org/onlinedocs/gcc-3.2/cpp/Pragmas.html
- at_a_remove 6y agoI have only ever dabbled in C, just to look at other people's code and occasionally when I really needed speed, so I am at what I would call a "Pretty Pathetic" level, able to recognize that I am looking at C. However, I look at old books on C, and then I look at this list, and I wonder if it would not have been helpful to, after mentioning that a function was banned, suggest what the replacement is, even as a comment.
- syncsynchalt 6y agoYou're not wrong. But a seasoned C developer looks at this list and nods along. (I'm a little out of practice, but I have war stories for most of these). It's likely that the authors of this list didn't think the comments would be worthwhile for the audience (git developers).
- whydoyoucare 6y agoI am so thankful git isn't forcefully including this header in every C language project and that we have a choice when using git! :-)
- lerax 6y agoYes, this is right. Any C decent programmer knows that functions are cursed.
- totorovirus 6y agoNow I am getting really curious whether other companies with supposedly strong engineering knew about sscanf issues.
- cynoclast 6y agolol, of course none of them have any documentation explaining why. Typical programmers. source: I'm a programmer
- userbinator 6y agoAt least they didn't ban memcpy()... Much like with all other forms of effective censorship, I see this as a quick short-term "fix" with hidden long-term costs[1]. IMHO this sort of anti-thinking just leads to even worse, more dogmatic and cargo-cult, programmers who know less and less about the basics and then go on to make even more subtle errors. Somehow the collective software industry has managed to propagate the notion that people are incapable of doing even basic arithmetic. Yet they think people are capable of creating complex systems with even more subtle behaviour? The justification would normally be because it's not directly affecting security. WTF. It's beyond stupid. The only C function I think should be truly banned is gets(), because it is actually impossible to calculate what size of buffer it needs. That is not true of any of the others on this list. [1] By short and long, I mean decades vs centuries.
- zX41ZdbW 6y agoThe similar list from ClickHouse repository: https://github.com/ClickHouse/ClickHouse/blob/master/base/harmful/harmful.c https://github.com/ClickHouse/ClickHouse/blob/master/base/ha...
- malkia 6y agoLet's use a loophole ;) - (strcpy)(a,b)
- robinduckett 6y agoIs there no linting software that can catch these kinds of issues? Like using strlen with sscanf like I've been hearing about lately?
- anovikov 6y agowhy is strncpy banned? what's wrong about it?
- known 6y agoJust banning is not fair; Include the alternatives;
- deleted 6y ago[deleted]
- jll29 6y agogets() and scanf() should be on that list due to potential buffer overflow.
- DyslexicAtheist 6y agoSome functions are missing which would normally cause a warning with most linters and static security analysis tools (e.g. the atoX family, mktemp, etc ...). Problem is most people I know don't run external linters (maintaining good linting rules is hard to scale in larger projects and in my >3 decades of writing C only few companies[0] I've seen managed the linting rules as part as their "definition of done"). While I think such rules are a good idea it only makes sense if it is done consistently and depends on how religiously the tooling (duct-tape and "process") enforces them (even so, you're still only one `#ifdef` away from undoing that "safety"). Having GCC[1] now support static analysis is a killer feature for this type of problem. On the other end of the spectrum we have Huawei which instead of linting their code is finding creative ways to trick auditing tools and hide such warnings from auditors: [0] https://news.ycombinator.com/item?id=22712338 https://news.ycombinator.com/item?id=22712338 [1] https://developers.redhat.com/blog/2021/01/28/static-analysis-updates-in-gcc-11/ https://developers.redhat.com/blog/2021/01/28/static-analysi... [2] https://grsecurity.net/huawei_and_security_analysis https://grsecurity.net/huawei_and_security_analysis
- shaggie76 6y agoOur forbidden functions header is similar; it's got about 30 functions including most of the str* family to enforce the use of our safer versions.
- synergy20 6y agoOK, so no strncpy, strncat etc, what are the alternatives used in git then? I'm a long-time C coder but I do not know what will be used to replace strncpy/strncat and all those gmtime/localtime/ctime/asctime.
- matt-attack 6y agoI used C many years ago so I’m quite out of it. What are the replacements for these? I would have thought these were all necessary.