7 ms·

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

by noname120 7y ago

arcturus17 7y ago
Read the manual!
wyldfire 7y ago
Collectively "because they're too easy to misuse (and even if used correctly, complicate audits)."
kevin_thibedeau 7y ago
None are properly length checked. strncpy() is for "binary" strings and can omit the NUL in addition to excessive zero padding large buffers.
calibas 7y ago
I believe they're all vulnerable to overflows and easy to exploit. https://pointerless.wordpress.com/2012/02/26/strcpy-security-exploit-how-to-easily-buffer-overflow/ https://pointerless.wordpress.com/2012/02/26/strcpy-security...
- saagarjha 7y ago
  > 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.
asveikau 7y ago
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.
- noname120 7y ago
  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.
  - noname120 7y ago
    I'd be interested in knowing which part of the comment you don't agree with and why.
  - philwelch 7y ago
    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.
    - noname120 7y ago
      Makes sense, thanks for taking the time to answer.
- magnetic 7y ago
  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).
  - asveikau 7y ago
    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.
noname120 7y ago
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.
- wruza 7y ago
  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.
  - noname120 7y ago
    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 https://ideone.com/h0g8F1
Someone 7y ago
Look at the commit comments (https://github.com/git/git/commits/master/banned.h https://github.com/git/git/commits/master/banned.h)
- chucksmash 7y ago
  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/c8af66ab8ad7cd78557f0f9f5ef6a52fd46ee6dd https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5e...
ohples 7y ago
Because it's git. It's obscure by design.
wincent 7y ago
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.
- MrStonedOne 7y ago
  Git is open source, it doesn't have its own developers, every developer is "gits own developers". They really should make this more explicit.
  - philwelch 7y ago
    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.
  - wincent 7y ago
    They probably don't want the source code to start resembling a Literate Programming example.
    - MrStonedOne 7y ago
      I don't think gatekeeping contributors and gatekeeping contributions are the same thing.
chucksmash 7y ago
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 :-).
ttaylorr 7y ago
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

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

arcturus17 7y ago

Read the manual!

wyldfire 7y ago

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

kevin_thibedeau 7y ago

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

calibas 7y ago

I believe they're all vulnerable to overflows and easy to exploit. https://pointerless.wordpress.com/2012/02/26/strcpy-security-exploit-how-to-easily-buffer-overflow/ https://pointerless.wordpress.com/2012/02/26/strcpy-security...

saagarjha 7y ago
> 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.

asveikau 7y ago

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.

noname120 7y ago
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.
- noname120 7y ago
  I'd be interested in knowing which part of the comment you don't agree with and why.
- philwelch 7y ago
  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.
  - noname120 7y ago
    Makes sense, thanks for taking the time to answer.
magnetic 7y ago
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).
- asveikau 7y ago
  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.

noname120 7y ago

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.

wruza 7y ago
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.
- noname120 7y ago
  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 https://ideone.com/h0g8F1

Someone 7y ago

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

chucksmash 7y ago
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/c8af66ab8ad7cd78557f0f9f5ef6a52fd46ee6dd https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5e...

ohples 7y ago

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

wincent 7y ago

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.

MrStonedOne 7y ago
Git is open source, it doesn't have its own developers, every developer is "gits own developers". They really should make this more explicit.
- philwelch 7y ago
  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.
- wincent 7y ago
  They probably don't want the source code to start resembling a Literate Programming example.
  - MrStonedOne 7y ago
    I don't think gatekeeping contributors and gatekeeping contributions are the same thing.

chucksmash 7y ago

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 :-).

ttaylorr 7y ago

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