Rarely have so many bytes^H octets have been wasted in the course of indulgence in such a pointlessly misguided indignation. Really, of all the software engineering problems that industrial reality presents in front of us, this one deserves the least attention. I am only thankful this is not some celebrity news of which there has been too many here in recent weeks, but that is not a very high standard to measure against.
Is this really important? Sure, the source may be slightly polluted, but it's not going to slow down the application once it's compiled.
Really, these sort of things are probably good in code: they make it explicit that you are dealing with char's, and not some other datatype. If left out, it may look like a bug, causing debugging hassles.
Agreed. I'm a strong proponent of lean codebases, but I still write `sizeof(char)`, because otherwise it's a dead ringer for a common mistake. Code should be concise only to the point that it doesn't become harder to read.
OK, I'm not exactly a professional C programmer, but among those who are: is the use of sizeof(char) here more important than the copy operation that is going to write the terminating null past the area actually malloc'ed?
Because that sounds suspiciously like "potentially exploitable" to me.
(1) sizeof is evaluated at compile time, so it would evaluate to (1 * strlen(params->text)), which the compiler would then optimize to just strlen(params->text). So, sizeof() is not exploitable.
(2) A terminating null character is placed at the end of strings, not at the end of allocated memory. malloc does not alter the memory before it returns a pointer to it; it is up to the programmer to ensure he is writing in the proper area.
The fact that it's strlen(params->text) is precisely why it's exploitable. If we intend to write strlen(params->text)+1 bytes and allocate less, we've set ourselves up for a buffer overrun.
No, you are right. The copying is a bad bug. Using sizeof(char) is mostly a coding style question.
The casting of the malloc() return value is something reasonable people can disagree about (by which I mean nerds can endlessly flame each other about). But here are some points:
* In C++, which Mozilla is written in, it is a compile time error to not cast, so most likely this blog post is just wrong. Conceivably, the file could have been a C file in an otherwise C++ project, but even in that case, it's at least understandable why the cast is there.
* If you have a macro that takes the type as a parameter:
In the blog's comment, someone pointed out the same thing but the author then dismissed it as, "I’m a C programmer, not a C++ programmer, so I wouldn’t know anything about that." But considering Mozilla is a C++ project, it's unfair to critique it out of context.
Maybe this is just the shitty programmer in me, but I'm going to go right ahead using sizeof(char) (I knew it's hardcoded at 1 before this, and I'll know after this), it appeals to the foolishly consistent programmer in me.
I actually disagree - while sizeof(char) might always be 1, code isn't just to communicate to a compiler, it's to communicate to another developer. When I see "sizeof(char) * strlen(foo)", I know that they want to allocate 'n' characters and that this is a string buffer: even though as a dev I certainly could've figured out, making code read like you think makes it far faster for others to interpret the intent of the code.
It's like when people define a 64MB buffer as something like:
#define BUFSIZE (64 * 1024 * 1024);
Even though you could've written 67108864, the former is far more comprehensible.
"[P]rograms must be written for people to read, and only incidentally for machines to execute." -- Abelson & Sussman, Structure and Interpretation of Computer Programs [1], strangely misquoted by Paul Graham [2]
It's also the main reason to use constants, even when magic numbers would do (e.g., use HOURS_IN_DAY instead of 24. It's not likely you'll ever have to change the number, but HOURS_IN_DAY is clearer for humans to parse).
This also applies to the bit in the article about coercing return values, which I disagree with. Funnily enough, he mentions that K&R also recommend coercion, even though they have a small note that it's not necessary any more. Still makes for clearer programs.
As with all rules like that they can be taken to DailyWTF levels of silliness.
I remember working on a Java codebase years ago where every string literal was defined in a Constants class - fair enough I guess. However, it had entries that looked like this:
public static String HTTP = "http";
public static String COLON = ":";
public static String SLASH = "/";
There's actually two problems with such code: one is that it doesn't actually make the code easier to read, but harder.
The other is more subtle: using constants like this is working at the wrong level of abstraction. The correct way to write this kind of code is to use a wrapper function which knows how to put together URLs, since there are lots of special rules to how URLs are put together (encoding, etc.) But a lot of people don't bother, and instead simply concatenate strings with a SLASH between them, and think that because they define the slash in some constant that makes the code correct.
I'd go even further with the issue. These constants do not offer _any_ abstraction or readability, and precisely this is the issue.
Naturally, I like constants like POPCORN_PRICE = 12 or whatever. They add meaning to meaningless numbers.
I am on the fence for HOUR_PER_DAY and MINUTE_PER_HOUR, because they never change, but they at least add a certain security if you chain them together. bar = lengthInSeconds * SECONDS_IN_MINUTE * MINUTES_IN_HOUR * DAYS_IN_YEAR. In this case you can simply match. Seconds - Seconds, fits. Minute - Minutes, fits. Hour - days... wait, doesn't fit.
However, if you look at PROTOCOL + COLON + SLASH + SLASH versus "http://, there is just nothing added.
> I am on the fence for HOUR_PER_DAY and MINUTE_PER_HOUR, because they never change, but they at least add a certain security if you chain them together. bar = lengthInSeconds * SECONDS_IN_MINUTE * MINUTES_IN_HOUR * DAYS_IN_YEAR.
Ideally, especially if you're using a strongly typed language, you should just use time-unit types (or at least macros or inlinable functions) for that: `fromSeconds(lengthInSeconds).toYears()`, lengthInSeconds.seconds.to_year, `SECONDS_TO_YEARS(lengthInSeconds)`, new TimeSpan(seconds=lengthInSeconds).toYears(), etc...)
The only reason to do that is if you want the compiler to check for typos: HTPP is caught, "htpp" isn't. It makes no sense for single letter constants of course.
and to avoid silly warnings from some static analysis software (e.g. findbugs telling you you are repeating a hardcoded string N times and you could use a constant)
Ridiculous! This is like saying that we shouldn't use parentheses because all C++ programmers should know order of operations! Not only that, but it's probably optimized out by the compiler (and if not, it is a quite insignificant speedup). Better to be safe using sizeof(x) than sorry ...
While the author is ridiculing those "silly" Mozilla devs I would like to ridicule the author of the blog. He seems like the person who would try to write "clever" code. Code that not only works correctly but teaches everyone _all_ the intricate operator precedence rules and all the possibly usages of bit operators, perhaps also code where they "cleverly" manipulate the call stack with asm inline instructions to demonstrate their assembly and C knowledge.
One thing I know -- that person is dangerous. How do I know? I was (perhaps still am) that person. It was a bad habit and I am still trying to get rid it. It is a veiled show of immaturity and arrogance.
Absolutely. I use ternary if when appropriate. But nesting ternary ifs is horrible, and nesting ternary ifs while discarding parentheses is malevolent.
Everyone is commenting on his point about sizeof(char). Does anyone have any comments on his recommendation to use expressions with sizeof rather than types?
It's no big deal, but it's just one less thing to change if you need to change the type. (I once had to go through some code changing longs to ints to make it run on a 64-bit machine. Were the mallocs written this way it might have been a bit easier.)
And although I agree that his sizeof(char) rage is overblown, is the author's point about the zlib code not reasonable?
I think it depends. In general, I prefer using expression, but there are cases where using sizeof is better (where better may be clearer, more maintainable, etc...).
I think casting malloc is useless in C (not only semantically, but from a readability POV), and is generally correlated with poor C programming. As other mentioned, mozilla is in C++, so that does not apply.
OK, define char. If it is a type built-in C - then of course sizeof(char) = 1. And there is no reason to write such posts.
The problem is people are confusing the definition and the usage, because this particular type "char" was supposed to hold a numerical representation of a symbol, which in case of a multi-byte encoding is not 1 byte anymore.
So, sizeof(char) is always 1, and what they trying to say is sizeof(numeric-representation-of-a-symbol) which is not a concern of the C language.
> A byte is always 8 bits, and therefore contains unsigned numbers 0 to 255 inclusive.
That definitely isn't true. Historically, bytes went from 5 to 16 bits, it was just the number of bits required to handle a character.
If you want to talk specifically about 8-bit bytes in an architecture-independent manner, use the word "octet" (which happens to be the word french people generally use).
`char` is a technically independent (though often related) datatype defined as being >= 8 bits by the ANSI standard and the definition of `sizeof(char) == 1` is an axiom of the ANSI standard, not the consequence of anything but itself, though the standard definitely seems to confound (confuse?) chars and bytes:
2 The sizeof operator yields the size (in bytes) of its operand, which may be an expression or the parenthesized name of a type. The size is determined from the type of the operand. The result is an integer. If the type of the operand is a variable length array type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an integer constant.
3 When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1. When applied to an operand that has array type, the result is the total number of bytes in the array.88) When applied to an operand that has structure or union type, the result is the total number of bytes in such an object, including internal and trailing padding.
I stand corrected and apologise - I genuinely mis-spoke (mis-typed?) myself. I do know that a byte is not (historically) necessarily 8-bits - indeed, I regularly used to program machines on which the basic memory unit was 12 bits.
Really, a better coding style is to use STL strings, containers and algorithms and never get down to the point where you have to care.
Coding at the bare-metal level is a very slow, tedious, error-prone and silly way to code unless you're doing, or have some external constraint that forces you to (and, no, efficiency isn't usually a reason not to use C++ or the STL, it almost always compiles to the same as using strcpy's and malloc's.)
The man is writing about C, not C++. C remains important. I agree that C++ and the STL are good toolsI don't agree that STL style string handling code will compile down to the same thing as C style string handling code.
Personally I don't agree with the post, starting from his analysis, of the first snippet of code:
group->text = (char *) malloc(sizeof(char) * strlen(params->text));
if (group->text == NULL) {
res = MP_MEM;
}
strcpy(group->text, params->text);
I don't think that the sizeof(char) or the casting of the pointer are of any danger and increase readability (we are not coding to show how well we know the standard).
The poster is right about the missing space in the malloc call for the NULL char, but I see a bigger threat calling a malloc with the size coming from a strlen: it can be really dangerous if somehow you miss a NULL terminator.
And again, same complain about the usage of strcpy. The standard gives us strncpy that put a limiting size in the copy operation.
_Personally I think that strcpy has to be avoided as a plague being a source of buffer overruns error as no other call in the C library._
It should be noted that not all C compilers are fully standard compliant. Many embedded system vendor compilers for example use incorrect operator precedence. Available library can also do things in non-standard ways.
This does not look like advice about coding for the real world, it seems to be about how to invest your time to maximise feelings of smug superiority.
This article only shows that some C programmers are not able to think at an higher level than whatever their compiler sees. They don't understand that a programming language standard is not a style guide.
54 comments
[ 6.8 ms ] story [ 137 ms ] threadReally, these sort of things are probably good in code: they make it explicit that you are dealing with char's, and not some other datatype. If left out, it may look like a bug, causing debugging hassles.
Because that sounds suspiciously like "potentially exploitable" to me.
(2) A terminating null character is placed at the end of strings, not at the end of allocated memory. malloc does not alter the memory before it returns a pointer to it; it is up to the programmer to ensure he is writing in the proper area.
The casting of the malloc() return value is something reasonable people can disagree about (by which I mean nerds can endlessly flame each other about). But here are some points:
* In C++, which Mozilla is written in, it is a compile time error to not cast, so most likely this blog post is just wrong. Conceivably, the file could have been a C file in an otherwise C++ project, but even in that case, it's at least understandable why the cast is there.
* If you have a macro that takes the type as a parameter:
then the casting is a good thing because it makes the compiler warn if you try to assign the result to the wrong pointer type.* It is true that if you forget to include the stdlib.h header, the cast will silence a useful warning about converting int to pointer.
It's like when people define a 64MB buffer as something like:
#define BUFSIZE (64 * 1024 * 1024);
Even though you could've written 67108864, the former is far more comprehensible.
"[P]rograms must be written for people to read, and only incidentally for machines to execute." -- Abelson & Sussman, Structure and Interpretation of Computer Programs [1], strangely misquoted by Paul Graham [2]
[1] http://mitpress.mit.edu/sicp/full-text/sicp/book/node3.html
[2] http://www.paulgraham.com/hp.html
This also applies to the bit in the article about coercing return values, which I disagree with. Funnily enough, he mentions that K&R also recommend coercion, even though they have a small note that it's not necessary any more. Still makes for clearer programs.
I remember working on a Java codebase years ago where every string literal was defined in a Constants class - fair enough I guess. However, it had entries that looked like this:
leading to code that looked like:There's actually two problems with such code: one is that it doesn't actually make the code easier to read, but harder.
The other is more subtle: using constants like this is working at the wrong level of abstraction. The correct way to write this kind of code is to use a wrapper function which knows how to put together URLs, since there are lots of special rules to how URLs are put together (encoding, etc.) But a lot of people don't bother, and instead simply concatenate strings with a SLASH between them, and think that because they define the slash in some constant that makes the code correct.
Naturally, I like constants like POPCORN_PRICE = 12 or whatever. They add meaning to meaningless numbers.
I am on the fence for HOUR_PER_DAY and MINUTE_PER_HOUR, because they never change, but they at least add a certain security if you chain them together. bar = lengthInSeconds * SECONDS_IN_MINUTE * MINUTES_IN_HOUR * DAYS_IN_YEAR. In this case you can simply match. Seconds - Seconds, fits. Minute - Minutes, fits. Hour - days... wait, doesn't fit.
However, if you look at PROTOCOL + COLON + SLASH + SLASH versus "http://, there is just nothing added.
Ideally, especially if you're using a strongly typed language, you should just use time-unit types (or at least macros or inlinable functions) for that: `fromSeconds(lengthInSeconds).toYears()`, lengthInSeconds.seconds.to_year, `SECONDS_TO_YEARS(lengthInSeconds)`, new TimeSpan(seconds=lengthInSeconds).toYears(), etc...)
One thing I know -- that person is dangerous. How do I know? I was (perhaps still am) that person. It was a bad habit and I am still trying to get rid it. It is a veiled show of immaturity and arrogance.
I once had to deal with code that did something like this:
I'm not kidding.I would say it's a case-by-case statement. Well, okay, more specifically, I would say that the use of a single ternary if is a case-by-case statement.
And although I agree that his sizeof(char) rage is overblown, is the author's point about the zlib code not reasonable?
I think casting malloc is useless in C (not only semantically, but from a readability POV), and is generally correlated with poor C programming. As other mentioned, mozilla is in C++, so that does not apply.
But it is rather a legacy nowadays. No one should use it.
The problem is people are confusing the definition and the usage, because this particular type "char" was supposed to hold a numerical representation of a symbol, which in case of a multi-byte encoding is not 1 byte anymore.
So, sizeof(char) is always 1, and what they trying to say is sizeof(numeric-representation-of-a-symbol) which is not a concern of the C language.
A byte is always 8 bits, and therefore contains unsigned numbers 0 to 255 inclusive.
A char is always defined to be the base size of the machine.
By definition, therefore, sizeof(char) is always 1.
However, a char is not always 1 byte. On some machines a char can hold the values 0..511, and on others 0..65535. (for example)
That definitely isn't true. Historically, bytes went from 5 to 16 bits, it was just the number of bits required to handle a character.
If you want to talk specifically about 8-bit bytes in an architecture-independent manner, use the word "octet" (which happens to be the word french people generally use).
`char` is a technically independent (though often related) datatype defined as being >= 8 bits by the ANSI standard and the definition of `sizeof(char) == 1` is an axiom of the ANSI standard, not the consequence of anything but itself, though the standard definitely seems to confound (confuse?) chars and bytes:
2 The sizeof operator yields the size (in bytes) of its operand, which may be an expression or the parenthesized name of a type. The size is determined from the type of the operand. The result is an integer. If the type of the operand is a variable length array type, the operand is evaluated; otherwise, the operand is not evaluated and the result is an integer constant. 3 When applied to an operand that has type char, unsigned char, or signed char, (or a qualified version thereof) the result is 1. When applied to an operand that has array type, the result is the total number of bytes in the array.88) When applied to an operand that has structure or union type, the result is the total number of bytes in such an object, including internal and trailing padding.
The other reply to my comment (http://news.ycombinator.com/item?id=1666345) gets this right.
Again, apologies.
Coding at the bare-metal level is a very slow, tedious, error-prone and silly way to code unless you're doing, or have some external constraint that forces you to (and, no, efficiency isn't usually a reason not to use C++ or the STL, it almost always compiles to the same as using strcpy's and malloc's.)
-- Ayjay on Fedang/coding
The poster is right about the missing space in the malloc call for the NULL char, but I see a bigger threat calling a malloc with the size coming from a strlen: it can be really dangerous if somehow you miss a NULL terminator.
And again, same complain about the usage of strcpy. The standard gives us strncpy that put a limiting size in the copy operation.
_Personally I think that strcpy has to be avoided as a plague being a source of buffer overruns error as no other call in the C library._
This does not look like advice about coding for the real world, it seems to be about how to invest your time to maximise feelings of smug superiority.