The shockingly obsolete code of bash
blog.erratasec.com
blog.erratasec.com
Sure, it could and perhaps even should be avoided in this particular case, but in general it's a useful and common C idiom. Compiler writers are aware of that and avoid issuing warnings if you parenthesize the offending assignment expression.
He also compares pointer values against NULL. If I were as uncharitable as he has been, I'd argue that this shows a 'shocking' lack of knowledge about how C works. In C, comparing against NULL is as 'bad' as comparing boolean expressions against true and false in other languages.
edited: be less cranky
I see no parentheses in that code.
There's also a corresponding git repository, also starting at 1.14.7: http://git.savannah.gnu.org/cgit/bash.git/log/?ofs=50
No idea about earlier revisions.
There was an important job to be done and Everybody was sure that Somebody would do it.
Anybody could have done it, but Nobody did it.
Somebody got angry about that because it was Everybody's job.
Everybody thought that Anybody could do it, but Nobody realized that Everybody wouldn't do it.
It ended up that Everybody blamed Somebody when Nobody did what Anybody could have done
GNU wants freedom for people to distribute code. But do they want us to have the freedom to understand their code? :-)
Technical debt is something you accrue when you deliberately violate a current best practice for gains elsewhere (usually meeting a deadline). It's not generally used to refer to code that's simply old.
Anyone 20 years from now looking at a snip of code you write today is going to have similar complaints.
Imagine if heartbleed happened and everyone just moved on because bugs happen, deal with it.
> k&r function headers
unusual, but totally valid, even in C99. a minor syntactical difference. not a style i'd use, but no different to the alternative. and if a static analyser would find this confusing, you probably shouldn't use it.
> global variables
i buy this. although your figure of "5" global variables is totally arbitrary. "0, 1, or infinite" principle.
> lol wat?
granted this for loop is arcane, but it's obvious what it does. note that the suggested alternative increments the index at the end of the loop body, etc. might not have been exploited there, but perhaps it's a style used somewhere else where it does matter, and it's been used here to maintain consistency.
regardless, it's probably better to use
const size_t len = strlen(env);
for(size_t i=0; i<len; i++)
//etc
> no banned functions
at this point i decided the author clearly does not know what he's talking about, and has joined a bandwagon of code-hate.
banned functions. unsafe memory moves. "what if the destination is too small for the result?". urgh. look at the code that was pasted. strcpy, obviously has pitfalls that can easily overwrite random bits of memory. but this code cannot possibly exhibit it. if you want to complain about safe use of strcpy for the functions potential unsafe-ness, well, you may as well argue that C is an unsafe language, as the runtime doesn't catch out-of-bounds accesses, etc.
> where to go from here
if you've got style complaints about bash's source, change it, and submit patches. blog posts like the one posted do not help anyone.
Maximum of 5 global variables? For loop syntax? "objective" measures of code quality?
This kind of code shows up in any legacy code base, and style isn't what leads to bad code. Bash had a parser bug, a bug which anyone could have introduced irrespective of style.
I suppose the objective measures he refers to are the static analysis he mentions in the next paragraph.
It is hardly a controversial that global state is bad, and that code should be clear rather than clever.
But clarity depends on context. Personally, in context of the C language, I find code like
while ((string = *env++))
Similarly, he complains that the loop index was called string_index instead of the 'far superior' i.
With C99, I'd agree. But in a C90 codebase, do you really want to have declarations like
int i,j;
int char_index, string_index;
Bash works, pretty well, for a lot of people. So nobody wants to touch it.
There are a whole lot of really important pieces of code that not a lot of people find sexy to work on.
(I suppose bash does not have a test suite.)
I'd be a lot more interested if someone had performed this analysis of Bash and then created some search thing that found similar programmin style in other tools - especially if they also found interesting bugs.
It's called static analysis and (judging by the tools we use) probably already ship with rules that would bark loudly at this code.
Just exactly five and no more, huh? Better rinse that number off, because we both know where you got it from.
...programmers who don't know C? None of this is particularly tricky C code.
While these functions can be used safely by careful programmers, it's simply better to ban their use altogether.
"If there's a chance something could be dangerous, ban it"? This feels like the same sort of reasoning that the government uses to deprive people of personal freedoms... all in the name of "security". It's not unexpected given these people are in the security industry, but sometimes I wonder what kind of world they really want...
The whole article has the feel to it of being written by someone to whom "best practices" are the gospel, and misses the extremely simple reason behind the bug: accidentally reusing a function that does both command parsing and execution to evaluate function definitions. The reason for this misuse could be because command execution is being conflated with evaluating function definitions, but more importantly, none of the complaints raised in the article matter; neither changing function definitions to C89+, removing global variables, rewriting the style of loops and renaming their variables, nor using "safer" string functions would've eliminated the bug. Without all of these, the same mistake could be made. It's a little amusing that one of the gripes is about variable naming, when I think one of the functions involved in the bug couldn't be named any clearer: parse_and_execute().
There is a certain allure to ticking off some list of "best practices" and thinking that it'll somehow solve everything, but as this bug shows, there's simply no replacement for thinking carefully about the code and what it does when implementing functionality.
The loop construct shows a coder who is obsessed with terse code, but knows their crap. The globals show somebody who doesn't care about sensibly organized code.
C doesn't have all the nice sugar that comes with newer or more high-level languages.
Sure, it would look nicer if we were able to write things like
for (char cc : stdin.chars) { ... }
for (string : iterator(env)) { ... }
for (int cc; (cc = getchar()) != EOF; ) { ... }
while ((string = *env++)) { ... }
IMNSHO the real issue is with the developers itself. A huge amount of them seem to be stuck in the 80s and anything invented in the field of software engineering is just ignored by them. Like there are plenty of programming languages now with saner string handling, but no, C89 or pre-ANSI C it is.
Though we will probably never know, I bet having legacy code isn't exclusive for open source projects.
Perhaps some of the very large companies who consume this code could contribute to the cost.
C is one of the few choices that is available on most computing platforms.
What I hope comes out of this is that Linux distributions standardize on a bare-bones shell for system-level and even application-level scripts, and more powerful shells are used only for interactive sessions.
But why? We have had UNIX shells since 1972 and they work and work well for what they were designed.
Also, the criticism of code style is unfounded. It was written ages ago to the standards of the day, tested to work and left alone as it should have (it works, works well and why touch it?). It may look strange to the people who have only started programming in the last couple of years and who would happily re-write every peace of working code to conform to their idea of coding standards and how things should be done.
It's called POSIX, aka ISO/IEC 9945:
http://www.unix.org/version3/iso_std.htmlInteresting fact: In early 60s there was a serious discussion about whether stack frames (called activation records back then) should be global or not.
Heck, most any project I've worked on that was started more than 3 years ago has obsolete code. If it was started before it was a near-universal habit to write tests, oh yeah we're scared to touch it.
But I agree with the author that this is a sign of things to come. Our digital empire is built on layers upon layers of bad code, and the economic cost to fix it would be enormous. And that's what's shocking. Expect things to get a lot worse before they (maybe?) get better.
Using obsolete technologies does not imply bad code, either.
One doesn't see blog posts complaining about the shockingly obsolete stuff in the Eiffel Tower, either (wrought iron & rivets. Why don't they rebuild it in welded steel, or, better yet, 3D print it in steel?), or of the over engineering in Gothic cathedrals (the ones still standing use more stone than necessary, now that we know how to do strength calculations)
Yes, if you want to make strong claims about the security or performance of bash, its code probably needs work, but there is plenty of newer stuff that also has its issues (for example, the typical mobile phone OS needs quite a few iterations to get its lock screen right, the average brand-new thing also has security issues, etc)
With no backward compatibility to think of, I can see several nice goals to have. It should use a modern language like python, bridging the unnecessary gap between scripting languages and shell. It should remove environment variables in favor of IPC like kdbus.
And while at it, removing the concept of tty would remove a huge obsolete concept from an bypass age.
I think this is utterly incorrect. Linux principle is "don't break user-space unless we really really have to". That includes dependence on undocumented behavior.
But without the source code, you can do neither of these things easily. The likelihood of the a) case occurring is lower by virtue of accessibility.
With the source code and the development history of the product in the open, we can at least see whether a misbehavior was intentional, or exploited logic questionable, and that there might have been a mistake in the original thinking. How easily would one have come to those conclusions if the source was not private? The response to the issue would have been very different, and I fail to see how if the source code wasn't open would it have somehow made it less likely a bug from the 80s wouldn't still be there.
I think it's even more important now than ever to keep source open to make sure bugs can be easily found; even if the benefits are marginal to discover, they are certainly better handled in the aftermath.