Security Errors in Code, Part 3

In Part 1 I said one of the main reasons the "Top 25 Most Dangerous Programming Errors" exist is lazy programming. In Part 2 I gave the following as an example.

void DisplayMsg (char *domain, char *file) {
  char *b, *p = "Processing  from ";
  int d = strlen (domain);
  int f = strlen (file);
  int l = strlen (p);

  b = malloc (d + f + l + 1);
  memcpy (b, p, 11);
  memcpy (b + 11, file, f);
  memcpy (b + 11 + f, p + 11, 6);
  memcpy (b + 17 + f, domain, d + 1);

  DisplayString (b);
}

I pointed out that not checking whether the malloc succeeded or not was an example of lazy programming. But I said there were more examples in that code. Here they are.

1. The function name. "DisplayMsg" could mean a lot of things, but this routine really displays a specific type of message built from a domain and file name. A better name would be "DisplayMsgFromDomainFile". "But," the lazy programmer exclaims, "that's a lot to type. I can code faster if I have less to type." Right, and how much typing you do is more important than good code. Other people will have to use and maintain this code, the more descriptive you are, the easier it is.

2. Single-character variable names. In Java, longer variable names increase code size, but not in C, so there's no reason to use single-character variable names. Besides, in Java, there are obfuscators that will convert descriptive variable names to single characters to reduce code size. In fact, think of it this way, if you use single-character variable names, you are, by definition, writing obfuscated code. In the above example, the single-character variables are not that bad, but lazy here means lazy elsewhere. Single-character variable names are harder to search for and it's harder for someone to know what they mean. And look at the variable lower-case L, is that a one or an ell?

3. No free. The buffer is allocated but never freed.

4. No comments. There's no description of what this function does, nothing about the arguments (is domain allowed to be NULL?). Although this is a simple routine, it will still take someone reading this a couple minutes to figure out if this routine does what they want it to do and how to use it. Comments would be easier to follow.

Documentation is important in code. People using your code and people updating and maintaining your code will need to know what the function is supposed to do. Descriptive function and variable names are part of documentation. Comments are part as well.

I have heard programmers declare that their code is so well-written, they don't need comments. Well, if you use descriptive function and variable names, that might be true to some extent. But suppose you're writing a multi-precision Montgomery multiply routine. We can know that's what the function does, but in the code itself, you do a shift, why? There's a 32-bit value called n0, what is that?

"If anyone is reading this code, they must know Montgomery multiplication first, they shouldn't have to learn it by reading this code." True, but what part of MontMul is a particular part of the code doing? Is that shift by 2 really a divide by 4 or a shift for some other reason? If there's a bug in the code, debugging will be made so much easier if there are comments.

Personally, I think it's bad to say that if you want to know what a function does, read the source code. There really are people who say that, usually in the O---l community. The problem is that there can be so many functions it would take forever to go through a .h file, look at a semi-descriptive name and then look at the source code for that routine (which contains single character variable names and calls other subroutines, so now I'll have to understand those functions to understand the one I'm investigating).

Programmers sometimes say, "I'll put the comments in later." Maybe that has happened but I have never seen it. I have actually put comments into other people's code after the fact, but I've never seen an original programmer do that. I have only seen programmers who put the comments in while coding, or never put them in.

I once worked with a guy who couldn't get his own code to work because he couldn't remember how to use it. He wrote the code 3 months prior, but he still couldn't remember which function to call in which order with which arguments. Looking at the source code did no good. He liked to use nondescriptive function names and single character variable names. I think the only comments he ever put into code was the copyright notice. And work he did three months ago was lost. The company paid him to do something and it was a waste of money. That's why your code needs to be well-written, easy to follow, and documented.

Lazy programmers say that so long as the code gets the right answer, that's all that matters. But you can have code that gets the right answer but has security flaws and is unmaintainable.

Craft your code so that it looks good, is not just correct. Write a description of what a subroutine does (every subroutine, not just the "important" ones), and what the arguments are before you even write the code. Comment the code as you write. Everyone says they know they should and sometimes they do, but so many never follow through. If you're too lazy to write function descriptions or comments, get out of the business.

Here's an analogy. Part of the grocery store clerk's job is to handle the coupons, either scan them or manually make the appropriate deductions. Any clerk who says, "Yeah, I should, but sometimes I don't so things move faster," is not doing their job. You wouldn't say, "It's up to the discretion of the clerk to decide if the customers' coupons are to be accepted. If they don't feel like it, then oh well." No, it's part of the job.

Making sure your code is documented right from the start, making sure your code is easy to read, making sure your code is completely correct (not just "it completes the task") is part of your job. If you don't do those things, you're not doing your job. If you don't want to do it, find another job.