strange parenthesis in small c program

Question

I'm writing a simple program in C:

int main(int argc, char** argv) {
    unsigned char* line = (unsigned char* ) malloc(0xFFFF);
    while (gets(line) > 0) {
        if (line[0] == 'l') {
            if (line[2]=='.' && line[3] == '.') {
                printf("forbidden path");
            } 
            unsigned char* res = (unsigned char* ) malloc(0xFFFF);
            unsigned char* cmd = (unsigned char* ) malloc(strlen(line) +
            1 + strlen(" | grep -v xml") + strlen("/home/files/"));
            strcpy(cmd, "ls ");
            strcpy(cmd + 3, "/home/boris/0servfiles/");
            strcpy(cmd + 3 + strlen("/home/files/"), line + 2);
            strcpy(cmd + 3 + strlen("/home/files/") + strlen(line + 2), " | grep -v xml");
            execwthr(cmd, res);
            printf("%s\n%s", cmd, res);
            free(cmd);
            free(res);
        } else if (line[0] == 'm') {
            if (line[2]=='.' && line[3] == '.') {
                printf("forbidden path");
            } 
            unsigned char res = (unsigned char* ) malloc(0xFFFF);
            unsigned char* cmd = (unsigned char* ) malloc(strlen(line) +
                1 + strlen("/home/files/"));
            strcpy(cmd, "mkdir ");
            strcpy(cmd + 6, "/home/files/");
            strcpy(cmd + 6 + strlen("/home/files/"), line + 2);
            execwthr(cmd, res);
            printf("%s\n%s", cmd, res);
            free(cmd);
            free(res);
        }
    }
    return (EXIT_SUCCESS);
}

There's one small problem. When I try to create a folder named "h" I get following:

m l
mkdir /home/files)l

What's wrong? thanks in advance!

**Debugging hint:** solve the string-manipulation and interacting-with-the-file-system parts of the problem separately. Indeed, those are two decent candidates for breaking this problem into functions. — dmckee --- ex-moderator kitten, Apr 23 '12 at 21:25
Seriously, stop now and get a good book on C. Just replace "database" with C in [this blog post](http://weblogs.asp.net/alex_papadimoulis/archive/2005/05/25/408925.aspx). — Adam Rosenfield, Apr 23 '12 at 21:26
At some point, you should explain what the `execwthr()` function does. Spelled thus, the only relevant link Google points at is this question. You should also forget that the function call `gets()` exists. — Jonathan Leffler, Apr 23 '12 at 21:39
I'm guessing from context that `execwthr()` works kind of like `system()`, except it writes the output into a buffer passed as its second argument. — , Apr 23 '12 at 21:53

score 2 · Answer 1 · 2012-04-23T21:31:17.907

You seem to be unaware of the strcat() function. Using it will simplify your code greatly.

Also, why are you writing C code to do a job that could be much more easily accomplished with a simple shell (or Perl) script? For instance, I believe your script would look something like this in Perl:

#!/usr/bin/perl
use strict;
while (my $line = <STDIN>) {
    chomp $line;
    my $cmd;
    if (my ($arg) = $line =~ m{^l (.*)$}) {
        $cmd = "ls /home/files/$arg | grep -v xml";
    } elsif (my ($arg) = $line =~ m{^m (.*)$}) {
        $cmd = "mkdir /home/files/$arg";
    }
    my $res = `$cmd`;
    print "$cmd\n$res\n";
}

Note that this code is untested, and it's (still) insecure. It is, however, much shorter and easier to read than your code. This is a sign that C is the wrong language for this task. :)

score 2 · Accepted Answer · answered Apr 23 '12 at 21:50

There are many problems here. I'll list a few, and solving them will probably make others more apparent.

You print out "forbidden path" when an unexpected path is presented as input. However, you continue on through the function, using that invalid path. You probably want to error out of the program completely when this happens.

The malloc() function can fail. When it does, it returns a NULL pointer. You never check the value returned by malloc, which means that your string manipulation code may be using an invalid pointer.

You shouldn't be using functions like strcpy(), as they give you no protection against buffer overflows. Use the "safe" versions instead (like strncpy).

The function gets() is painfully unsafe and is deprecated. Use other standard library I/O functions instead.

This code:

strcpy(cmd, "ls ");
strcpy(cmd + 3, "/home/boris/0servfiles/");
strcpy(cmd + 3 + strlen("/home/files/"), line + 2);

is probably not doing what you want it to do. The second line adds a long string to the buffer, but the third line only skips forward far enough to accommodate a much shorter string. That means that your third strcpy here will partially overwrite the string written by the second strcpy.

Code like this:

strcpy(cmd, "mkdir ");
strcpy(cmd + 6, "/home/files/");
strcpy(cmd + 6 + strlen("/home/files/"), line + 2);

is error-prone and full of magic numbers. If I'm interpreting what you're doing correctly, you can combine this into one call that makes a lot more sense: sprintf(cmd, "mkdir /home/files/%s", line + 2);.

As others have mentioned, you seem to be re-implementing the standard strncat() function. You would be wise to use the standard version instead. If you are dealing with hard-coded strings like this, I'd even recommend using snprintf().

The following line is wrong:

unsigned char res = (unsigned char* ) malloc(0xFFFF);

You're assigning a pointer to a char, which almost certainly isn't what you want.

You dynamically allocate the memory for line, but you never free it.

`malloc` failing is the least of his worries. And [`strncpy` does not necessarily null-terminate its output](http://stackoverflow.com/questions/1453876/why-does-strncpy-not-null-terminate). — Adam Rosenfield, Apr 23 '12 at 21:54

score 1 · Answer 3 · answered Apr 23 '12 at 21:24

This code is pretty bad. Please refer to the strcat() function.

I see numerous other problems as well. For example, if the code determines that a "forbidden path" was entered, it then goes on to perform the task anyway!

You would benefit from learning how to use the debugger to step through this code and see what it actually does. That would also allow you to be able to answer your original question as well.

score 0 · Answer 4 · answered Apr 23 '12 at 23:33

Basically all of your ugly code could be replaced by a single call to snprintf with the right arguments. Don't bother learning strcat; while, as others have said, it will be easier than what you're doing, it also makes it easy to write vulnerable code. Just use snprintf for all your string-producing needs and your code will be clean, simple, and even has a decent chance of being bug-free.

strange parenthesis in small c program

4 Answers4