2
\$\begingroup\$

I wrote a program in C that simulates the touch command from linux. I would like to ask you to criticize my implementation, to find bugs in particular

#include<sys/types.h>
#include<sys/stat.h>
#include<stdio.h>
#include<stdlib.h>
#include<fcntl.h>
#include <sys/dir.h>
#include <sys/param.h>
#include <errno.h>
#include <dirent.h>
#include <string.h>
#define MAX_PATH 1024

void  touch (const char *dir,const char *fileName, char path[4096]) {
     // "dirent" object provides us with an internal function
    struct dirent *d;
    DIR *dh =opendir(path);
    // Error Handling Block
    if (!dh){
        return ;
        if (errno== ENOENT){
            perror("Directory does not exist");
        }
        else{
            perror("Unable to read the  filel");
        }
        exit(EXIT_FAILURE);
    }
    // Char array for newpath
    // ls same logic 
    char newpath[4096]="";
    strcat(newpath,path);
    strcat(newpath,"/");
    strcat(newpath,fileName);
    printf("%s\n",newpath);

    // Creating file on the specified path
    
    int f2= open(newpath, O_CREAT | S_IRUSR | S_IWUSR);
    
    while ((d= readdir(dh))!= NULL)
    {
        if (d->d_name[0]=='.'){
            //cwd
            continue;
        }
        printf("%s\n",d->d_name);
       // Just calling LS to show that the file has been created
      

    }
    
}

int main(int argc,const char *argv[]){
   
    // Two args --> ./touch , filename [to be created]
    if(argc==2){
        touch(".",argv[1],".");
    }
    else{
       
        perror("Enter valid Command");
    }
    return 0;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Whatever you think touch does, this isn't even a superset of the actual command. If it works as you intend then there are no 'bugs', but its not event remotely following the specification. Without knowing the specification, how can we judge what is a bug? \$\endgroup\$ Jun 5, 2022 at 15:08
  • 1
    \$\begingroup\$ What is the purpose in defining MAX_PAtH? It's unused, and regardless the majority of systems (including GNU/Linux) do not enforce a maximum limit on pathnames. \$\endgroup\$
    – JohnScott
    Jun 5, 2022 at 23:05

2 Answers 2

4
\$\begingroup\$

There is no need to deal with directories

There is no reason to pass touch() both a dir and a fileName argument, especially not if dir is going to be "." anyway. Just call open(fileName, ...) directly. Then check for the return value of that open() call, if it didn't succeed you can check errno to see why it failed.

Limit the responsibilities of each function

Your touch() does many things apart from actually touching the file. In particular, it is also doing the equivalent of ls. You should keep the function small and concise, only doing what its actual purpose is. If you do want to test whether the function is working correctly, do that in another part of your program. You might even want to consider using a unit testing library for that.

I would rewrite touch() like so:

bool touch(const char *filename) {
    int fd = open(filename, O_CREAT | S_IRUSR | S_IWUSR);

    if (fd == -1) {
        perror("Unable to touch file");
        return false;
    }

    close(fd);
    return true;
}

Now its only job is touching the file, and if it fails it will report the error, both to the terminal and as a return value, so the code that calls it can also check if it succeeded or not.

Also note that there is a lot less to worry about in this function. No need to allocate arrays for paths, no building paths, no directory scanning. It is much easier to ensure a small function like this is efficient and bug free.

Exit with a proper error code

Your program will always return with a zero exit code, even if it fails in some way. This is problematic; consider that your version of touch gets used in a shell script doing many things. You want the script to know something went wrong, so it doesn't continue running in an incorrect state. Make sure that you do return EXIT_FAILURE from main() if something went wrong.

Handle multiple arguments

The standard touch command allows you to touch multiple files in one go. You can easily support that in your program by adding a for-loop that goes through all the command line arguments.

\$\endgroup\$
1
\$\begingroup\$

Avoid overflow

With exceptional long names, below overflows the buffer.

char newpath[4096]="";
strcat(newpath,path);
strcat(newpath,"/");
strcat(newpath,fileName);

Instead,

int len = snprintf(0, 0, "%s/%s", path, fileName);
assert(len > 0); // snprintf() returning a negative is not reasonably possible here.

// Use a VLA or allocate memory.
char newpath[len + 1u];
len = snprintf(newpath, sizeof newpath, "%s/%s", path, fileName);
assert(len > 0); // snprintf() returning a negative is not reasonably possible here.

If wanting to stay with a fixed size buffer, at least test for sufficient room.

char newpath[4096];
int len = snprintf(newpath, sizeof newpath, "%s/%s", path, fileName);
if ((unsigned) len >= sizeof newpath) {
  fprintf(stderr, "Name too long \"%s/%s\".\n", path, fileName);
  exit EXIT_FAILURE;
}
\$\endgroup\$
5
  • \$\begingroup\$ The most correct fix is rather to input sanitize argv. Can be as easy as a memchr search for \0. Then you need not concern yourself with potential overflows later on. \$\endgroup\$
    – Lundin
    Jun 9, 2022 at 11:48
  • \$\begingroup\$ @Lundin 1) Why memchr() search for \0 vs. strlen()? 2) "not concern yourself with potential overflows later on" assumes fileName has a fixed size. Yet your point is good as by knowing the path length ahead of time (a one time cost), the sizing test only depends on strlen(fileName) - a smaller problem. \$\endgroup\$ Jun 9, 2022 at 12:14
  • \$\begingroup\$ With memchr you can set a maximum length. \$\endgroup\$
    – Lundin
    Jun 9, 2022 at 12:58
  • \$\begingroup\$ @Lundin True, yet each argv[] from main() is a string (expect the last is a null pointer) of length up to SIZE_MAX. It then appears to have no advantage of memchr() vs. strlen(). Ah, now I see, no need to find length past 4096? \$\endgroup\$ Jun 9, 2022 at 13:02
  • \$\begingroup\$ Yes and also various classic exploits involve passing huge strings as arguments. For example, imagine how strlen will behave when fed a string which is longer than SIZE_MAX. One possibility is unsigned wrap-around. Which could perhaps be used to trick strlen into thinking that the argument isn't huge. \$\endgroup\$
    – Lundin
    Jun 9, 2022 at 13:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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