Hacker News Reader

Comment by noname120 | Hacker News Reader

Why is there no brief explanations in this code why each function is banned?

A C programmer reading this list knows exactly why they are there. It is not at all a controversial list.

Edit: I guess some down voter doesn't believe me but it continues to be true. They are all string functions. Most of them do not take an output buffer size, so a source string exceeding the destination buffer will overflow. Others, like strncpy, take an output buffer size but will not null terminate when exceeded, so the buffer size must be reduced by 1 by the caller and potentially manually terminates, which is too easy to not consider. But any C programmer with significant experience already knows these pitfalls. So it's not controversial.

That's gatekeeping. If a junior developer starts working on this project they might bother a more senior developer who has better things to do with their time, or inline the definition of one of the functions out of frustration.

I'd be interested in knowing which part of the comment you don't agree with and why.

Git, like the Linux kernel, is essential, basic infrastructure that everybody uses and that needs to remain reliable. It's not a volunteer training project for dilettantes and neophytes. Gatekeeping is a deliberate part of how these projects are run and the results speak for themselves.

Makes sense, thanks for taking the time to answer.

You explained it in 2 lines that would make for great comments.

It seems useful to educate C programmers who don't have "significant experience" because, well, by definition, not every C programmer has "significant experience" and they are the ones who would benefit most from learning this.

It's not about being controversial: nobody's going to disagree with your explanation (it is what it is - it's not an opinion).

I don't remember how I learned this. But it happens pretty naturally working with C a lot and doing things like reading documentation and code. To be clear this is not elitism or something of the sort, I am not at all pessimistic that new people won't learn it. It just needs to happen in its due time.

So I would say... What good is a verbose comment to explain? Somebody who doesn't instantly understand why it's banned can read the big warnings on the manpage, or they can google it and land on good explanations on sites like Stack Overflow, or they can look at the function signatures and come up with a correct guess. Then boom. They know. And from there they can assess other interfaces and see if they suffer the same weakness.

This ability tends to come from simple exposure.

If every C programmer knew, then there would be no need to ban them

I actually wonder why they do it.

Linkers already warn for this stuff. Doing it this way requires every source file to include banned.h, which I would guess is done by including it from some other common header, but that's not fool proof either.

That's not true. People don't staunchly avoid situations that have pointless sharp edges. Putting up caution tape, despite everyone being aware of the danger, is very often a useful activity.

FYI that response insinuates that noname120 is not a C programmer...

Look at the commit comments (https://github.com/git/git/commits/master/banned.h)

Thanks for posting these. My company has a similar list of banned C calls so I was already familiar with the rationale. The initial patch[0] in particular is eye opening though. 157 lines of rationale and exploration of alternatives for 22 lines of uncontroversial code.

[0]: https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5e...

There are. It was mentioned that they are available in the 'commit comments' on 'banned.h', but you can find them by blaming down to the original commit which introduced each line.

Here are some of them:

  - strcpy()'s rationale: https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5ef6a52fd46ee6dd

  - strcat()'s rationale: https://github.com/git/git/commit/1b11b64b815db62f93a04242e4aed5687a448748

  - strncat()'s rationale: https://github.com/git/git/commit/ace5707a803eda0f1dde3d776dc3729d3bc7759a

  - sprintf()'s rationale: https://github.com/git/git/commit/cc8fdaee1eeaf05d8dd55ff11f111b815f673c58

I believe they're all vulnerable to overflows and easy to exploit.

https://pointerless.wordpress.com/2012/02/26/strcpy-security...

> As many already know, strcpy(3) is a very unsafe function call in the C library (you should always use strncpy(3) unless you can be exactly certain about the number of bytes you’re going to copy beforehand).

strncpy pretends to be safe but fails to NULL-terminate and has other performance issues that generally lead me to believe it’s a security placebo. I would recommend using snprintf instead.

Whenever you write, you have to consider your audience.

I'd assume there are no comments explaining this because for the intended audience, it is self-evident. "People browsing Hacker News on a Sunday" was probably not the intended audience :-).

Just to make it clear. I'm aware of the fact that these functions have security implications. Nonetheless when such decisions are made, I believe it's important to explain them—at least briefly—directly in the code itself.

With the way they are banned, you see it only as a compiler warning on implicit declaration of function (or at linking stage, if warnings are suppressed; idk what exact build process is). Afair, there is no evidence of banned.h in an error trace. So you have to look it up by yourself anyway.

They could rename them as “sorry X is unsafe and creates too much trouble” maybe, but in today’s internet “X considered harmful” is a common search query that everyone in the field is expected to know.

Fair point. It seems that more and more projects use "-Werror" which causes the compilation to stop and show you exactly where the error comes from[1]. Second point is that searching for "is_a_banned_function" in the code directly points to this file.

[1] https://ideone.com/h0g8F1

Because Git's own developers know to use git-blame, git-show, and write good commit messages. Comments are not the only place to store meta-info about why code is the way it is.

Git is open source, it doesn't have its own developers, every developer is "gits own developers". They really should make this more explicit.

Of course Git has its own developers, just as the Linux kernel does. There's no moral, technical, legal, or organizational obligation for the maintainers of any open source project to accept or solicit changes from randos.

They probably don't want the source code to start resembling a Literate Programming example.

I don't think gatekeeping contributors and gatekeeping contributions are the same thing.

None are properly length checked. strncpy() is for "binary" strings and can omit the NUL in addition to excessive zero padding large buffers.

Collectively "because they're too easy to misuse (and even if used correctly, complicate audits)."

Read the manual!

Because it's git. It's obscure by design.

Reply on news.ycombinator.com