Hacker News Reader

Comment by ptrain | Hacker News Reader

For those of us who can't spot this stuff as easily - is this detrimental, or simply out of style? Is there a recommended guide or body of source code this is "modern" C?

View on HN

Here's a 2011 guide to modern C: [1] Discussed on YC last year.[2]

The 1970s string functions, such as "strcpy", that don't have a destination length were deprecated years ago, but they've never been removed from libraries. Microsoft's C compiler has warned about this for a decade.[3] This is one of the major causes of buffer overflows in C, and a large number of exploits involve it.. There's a CERT advisory from Homeland Security about "strcpy".[4] Seeing a "strcpy" in new code is a big red flag. If you see that in an interview, don't hire that programmer. They're dangerous.

Since C99, you've been able to write

    for (int i = 0; i < 10; ++i) ...

in C code, and that was in C++ from the early years. The trend in programming is strongly towards declaring and initializing variables at the same time. Remember, in C, local variables declared but not initialized have junk values until assigned. The "all variables declared at the top of a function" style is obsolete. Pointer variables declared without initialization are especially bad.

There's a long and painful history of classic C bugs, and the newer versions of C help, just a little, to avoid them.

[1] http://icube-icps.unistra.fr/img_auth.php/d/db/ModernC.pdf [2] https://news.ycombinator.com/item?id=9018247 [3] http://stackoverflow.com/questions/4012222/c-strcpy-gives-a-... [4] https://buildsecurityin.us-cert.gov/articles/knowledge/codin...

  > Since C99, you've been able to write
  >
  >    for (int i = 0; i < 10; ++i)

To be fair, Microsoft's compiler only began supporting that recently, so for cross-platform compatible code, people often declared the index outside the loop.

Playing devil's advocate: local variables declared but not initialized before being used will always generate a warning (which can be turned into a hard error) on modern optimizing compilers like gcc or clang.

And having all variables declared at the top of the function can make it easier to visualize how much stack space the function is using, which can be useful when your stack is limited to a few kilobytes.

IMHO strcpy() is perfectly fine if you know the length of the source, and ensure the destination is always big enough. strcpy() will always write strlen(src)+1 bytes, nothing more and nothing less. That said, code should be written so those facts are obvious, and documentation is a preferred way to show that. (Being explicit about whether null-terminators are included or not in length specifications is one thing that seems to be often overlooked, for example.)

Microsoft's _s functions don't help if you don't know what the lengths should be, and if you do, they just make it more confusing.

Yup, I'm gonna stick my neck out here and say I quite like strcpy()

In good code it spells out in one glance that there exists a guarantee on the size of the destination buffer.

Supplying the string's length just brings redundant code and, with it, ambiguity.

Discouraging strcpy() use is fine and understood; I'm aware of all the caveats and bugs and security implications.

But the kind of black-and-white thinking that says "always use strncpy instead of strcpy" is bad; the idea that truncating a string magically absolves us of any security implications or the need for exit paths.

And then let's look at how much code has made a mess of strncpy() whilst thinking it was doing the right thing.

I can probably count on one hand the number of times I've wanted to copy a string but been happy for it to be quietly trimmed, even in extreme cases. Whereas anything from alloca to flexible array member, or copying a string back to its original buffer, are all very appropriate use of strcpy().

Declaring variables at the beginning of such a small function is certainly not out of style. IMHO, it's not detrimental. In fact, I find it quite useful (i.e. just by reading the declarations, I can infer the expected flow of the function -- fd will be used to open a device file, from which ifr is going to be populated through a call to read() or an ioctl). However, it's largely a question of style.

Using strcpy is a big no-no -- it's both detrimental, and out of style. Its manpage gives this warning (on FreeBSD -- the one you'll get if you type man strcpy may differ, but all man pages have had a similar warning since 1990something):

> The strcpy() function is easily misused in a manner which enables malicious users to arbitrarily change a running program's functionality through a buffer overflow attack.

That's because strcpy(dst, src) will basically do something like this:

    int i = 0;
    while (src[i] != '\0') {
        dst[i] = src[i];
        i++;
    }

(the real implementation is usually more terse for reasons I won't go into right now, but this is what it basically does in more verbose terms).

without bothering to check if dst[i] is not beyond the end of dst. This allows you to write past the end of the buffer that holds dst, and who knows what's there...

Nowadays, all that usually happens (on sane operating systems running on machines with a MMU) is that your application crashes due to something called ASLR. It used to be a big problem before that, though, and it still is (on insane operating systems, on machines that don't have a MMU and so on).

Either way, it's not a good idea, and it's considered to be a pretty big code smell. strncpy is the encouraged version. strlcpy is the sane one, but it's not available on all platforms (and even when it is, it's not always sanely implemented).

Strncpy is not used correctly either.

  strncpy(ifr.ifr_name, dev, IFNAMSIZ);

will not necessarily NULL terminate ifr.ifr_name for all values of dev variable. NULL terminated input is assumed later when ifr.ifr_name is used in strcpy.

In this case, it seems the ioctl call will do the null termination & in fact, the kernel itself uses strcpy for that field (presumably OK since the device structure has a null terminated name):

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux....

> I can infer the expected flow of the function

Old C code declared variables at the beginning of a function because it made compilers easier to implement, in a single pass. If you interleave variables and statements, then calculating the amount of stack space needed for the function call requires multiple passes. Knowing this, defenses of "variables at the top" often seem like rationalizations to me.

There's a reason that other programming languages don't share this convention, and that's because compilers had evolved enough in sophistication to allow interleaved variables and statements from their beginnings; and they had no legacy code bases anchoring their code style, like C does.

Declaring variables at the top is suboptimal compared to declaring them at their usage. It requires less working memory to understand, and it's fewer lines of code, and the resulting code can often be easier to understand due to context. Standalone variables typically either require more documentation to make sense, or else they don't make sense until you see them used in context.

The C language has a lot of bad conventions and semantics that have led to thousands of bugs over the years. Variables whose scope span entire functions is one of them. There's no articulable advantage and the downside is greater demands on working memory, and greater risk of defects (variables being visible before they're initialized or outside places where they make sense). Rust is on the right track allowing variable lifetimes to be defined and managed in a precise way.

> Knowing this, defenses of "variables at the top" often seem like rationalizations to me.

Like I said, it's a matter of style. Defenses of one or another that span more than one paragraph often seem like a waste of time to me.

Reply on news.ycombinator.com