4
\$\begingroup\$

I'm learning C with K&R 2nd Ed. I just completed exercise 1-20 (which is the detab program). I was hoping to get some feedback on my work. I want to make sure that I am taking a good C approach and that my knowledge in other languages isn't bleeding in.

/*
Exercise 1-20 in K&R 2nd Edition
detab: clarified here: http://stackoverflow.com/questions/7178201/kr-exercise-1-20-need-some-clarification
Written by Z. Bornheimer (provided as is without warranty).
*/

#include <stdio.h>

#define MAXLEN 10000
#define TABSTOP 4

int detab(char c, char str[], int i);

/* calls detab with appropriate data */
main()
{
    int i = 0;
    char c, str[MAXLEN];
    while ((c = getchar()) != EOF)
       i = detab(c, str, i);
    printf("%s\n", str);
    return 0;
}

/* replaces tabs w/ spaces in accordance to TABSTOP */
int detab(char c, char str[], int i)
{
    if (c == '\t')
        do
            str[i++] = ' ';
        while ((i % TABSTOP) != 0);
    else
        str[i++] = c;
    return i;
}
\$\endgroup\$
6
\$\begingroup\$

I don't think this program is correct, according to my interpretation of tab stops. One problem is that the column count should reset to 0 after every carriage return or newline character encountered.

Your program is vulnerable to buffer overflow. This may be acceptable for a beginner, as long as you acknowledge the problem, but this code should never be put into production use. (You may think that your buffers are generously sized, but a malicious attacker will scoff at whatever limit you choose. There is no substitute for proper bounds checking.) One simple strategy you could use is to minimize the use of a buffer by printing output on every call to detab() — your buffer would not need to be much larger than TABSTOP bytes.

In your detab() function, you want to treat i as an in/out parameter (i.e., the function alters the parameter and passes it back to the caller). It is customary to accomplish this in C by passing a pointer to i, like this:

void func(int *in_out_param) {
    while (0 != *in_out_param % 4) {
        (*in_out_param)++;
    }
}

void caller() {
    int i = 2;
    func(&i);
    printf("%d\n", *i);  /* prints 4 */
}

Your variables are cryptically named. While short variable names are acceptable and even encouraged as, say, dummy variables for iteration where their purpose is obvious, they should not be used as a matter of habit. In particular, it is important to give descriptive names to function parameters, since they help prevent accidental misinterpretation by the function's users. I would suggest this function interface:

/**
 * Converts input character c at column col into a string, with the output
 * placed in buf.  If c is a tab character, it is expanded into the appropriate
 * number of spaces.  The buffer size should be at least one more than tabwidth.
 */
void detab(unsigned int tabwidth, char c, unsigned int *col, char *buf, size_t bufsize)

For best performance, avoid reading one character at a time using getchar(). For this application, I recommend fgets(), which reads one line at a time (or up to the buffer size or until end-of-file, whichever is shortest). Admittedly, that does complicate the solution quite a bit.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the tips. I'm not sure what you mean by the code is vulnerable to buffer overflow or how to fix it. Could you explain that a little? Also, I'm keeping to what's been taught thus far (except for the do-while loop) and not using pointers yet. Lastly, the problem was kind of vague, so I wasn't sure if I should treat '\n' & '\r' as characters that forced (i % TABSTOP) == 0. \$\endgroup\$ – Z. Bornheimer Sep 24 '13 at 14:42
  • 1
    \$\begingroup\$ You've allocated 10000 bytes for str. However, if the output exceeds 10000 bytes, you blissfully write past the 10000-byte limit, into memory that you have not requested. That's called a buffer overflow. If you're lucky, you get away with the transgression and nothing bad happens. If you're less lucky, the OS detects the error and crashes your program. If you're unlucky, the user is a hacker who has crafted the input to write specific bytes that also act as machine code, tricking the computer into executing something completely unintended. \$\endgroup\$ – 200_success Sep 24 '13 at 14:57
2
\$\begingroup\$

This code is wrong:

char c;
while ((c = getchar()) != EOF)

Recall the following facts about C:

  1. The type char is either signed char or unsigned char (but it's implementation-defined which).
  2. EOF "expands to a negative integral constant expression"
  3. getchar() returns "the next character (if present) as an unsigned char converted to an int" precisely so that it can distinguish EOF (which is negative) from all valid characters (which are non-negative)

So there are two possible ways this could go wrong:

  1. If char is unsigned char, then when getchar() returns EOF, this gets converted to some unsigned char value (for example, -1 may be converted to 255) when stored in c, and so this will never compare equal to EOF and the loop will never terminate.

  2. If char is signed char, then there is a character that can be returned by getchar() which will be converted to the same value as EOF when stored in c. (for example, character 255 may be converted to -1 when stored in c, and so compare equal to EOF and terminate the program).

So you must replace:

char c;

with

int c;

(This is one of the classic C-language traps, by the way: here it is in the C programming language FAQ.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.