Thread: free() usage

  1. #1
    Registered User
    Join Date
    Mar 2002
    Posts
    87

    free() usage

    I'm having a problem with a loop in my code. Basically what I'm trying to do is to have a "/" and the filename concatenated to the end of a directory path that the user provides at the command line. So for example the user inputs "/usr" then when the program scans through each entry in this particular directory, it should be able to get the full physical path such as "/usr/file1". Instead what happens is the following.

    /usr/.//
    /usr/./..//
    /usr/./file1///
    /usr/./../file1/file2////

    Of course this is not what I want, I know why this is happening because it's thanks to this little piece of code.

    Code:
    if(S_ISDIR(sbuf.st_mode)){ /* Is a directory */
            count = scandir(argv[1],&direntp,NULL,alphasort);
            if(count){
              for(index=0; index<count; ++index){
                printDetails(direntp[index]->d_name,argv[1]);
                free(direntp[index]);
              }
            }else{
              fprintf(stderr,"Can't open %s\n",argv[1]);
              exit(EXIT_FAILURE);
            }
    }
    Every time it encounters an entry it loops through and passes both the entry name (direntp[index]->d_name) and the path that has been provided (argv[1]). Also this code in the printDetails function plays a part to the bug.

    Code:
    }else{
        strcat(path,"/");
        strcat(path,filename);
        fprintf(stderr,"Path: %s\n",path);
        fprintf(stderr,"Can't stat %s\n",curdir);
        return;
      }
    Now the thing is i've also done the concatenating before with full success from the following code.

    Code:
    /*Concatenate dir names*/
      curdir = workdir();
      strcat(curdir,"/");
      strcat(curdir,filename); 
    
      /*stat the final dir name*/
      if(stat(curdir,&sbuf) != -1){
        if(S_ISLNK(sbuf.st_mode)){
          if(readlink(curdir,buf,bufsize) != -1){
            if(lstat(buf,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
          }else
            fprintf(stderr,"Can't readlink %s\n",filename);
        }else if(S_ISREG(sbuf.st_mode)){
          if((fdes = open(curdir,O_RDONLY)) != ENOENT){
            if(fstat(fdes,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
            close(fdes);
          }else{
            fprintf(stderr,"Can't open %s\n",filename);
            return;
          }
        }
      }else{
        // previously listed code
      }
    And just to make sure i haven't missed anything heres all the code. In case you haven't guessed the following code should perfrom something similar to the "ls -al" unix command.

    Code:
    #include <stdio.h>
    #include <sys/types.h>
    #include <fcntl.h>
    #include <dirent.h>
    #include <stdlib.h>
    #include <time.h>
    #include <errno.h>
    #include <string.h>
    #include <sys/stat.h>
    #include <pwd.h>
    #include <grp.h>
    #include <unistd.h>
    
    #define SIZE 256
    
    /*prototypes*/
    char* workdir(void);
    void printDetails(char* filename,char* path);
    int main(int argc,char** argv);
    
    char* workdir(){
      size_t size = FILENAME_MAX;
      while(1){
        char* wdbuf = (char*)malloc(size);
        if(getcwd(wdbuf,size) == wdbuf)
          return wdbuf;
        free(wdbuf);
        if(errno != ERANGE){
          fprintf(stderr,"Out of range\n");
          return 0;
        }
        size *=2;
      }
    }
    
    void printDetails(char *filename,char *path){
    
      struct passwd *my_passwd;
      struct group *my_group;
      struct stat sbuf;
      struct tm *strtime;
    
      int fdes;
      unsigned char type ='?';
      unsigned char usr[] = "---";
      unsigned char grp[] = "---";
      unsigned char oth[] = "---";
      unsigned char uid[9], gid[9];
      unsigned char mod[13];
      mode_t mode;
      char* curdir;
      char* buf;
      size_t bufsize =FILENAME_MAX;
      time_t now;
      int year;
    
      /*Concatenate dir names*/
      curdir = workdir();
      strcat(curdir,"/");
      strcat(curdir,filename); 
    
      /*Fails because it uses the incorrect
        directory path. Eg. if /root was
        passed as the argument then sub dirs
        within root would try to be stat'd with
        for example /root/ls/temp instead of
        /root/temp . */
    
      /*stat the final dir name*/
      if(stat(curdir,&sbuf) != -1){
        if(S_ISLNK(sbuf.st_mode)){
          if(readlink(curdir,buf,bufsize) != -1){
            if(lstat(buf,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
          }else
            fprintf(stderr,"Can't readlink %s\n",filename);
        }else if(S_ISREG(sbuf.st_mode)){
          if((fdes = open(curdir,O_RDONLY)) != ENOENT){
            if(fstat(fdes,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
            close(fdes);
          }else{
            fprintf(stderr,"Can't open %s\n",filename);
            return;
          }
        }
      }else{
        strcat(path,"/");
        strcat(path,filename);
        fprintf(stderr,"Path: %s\n",path);
        fprintf(stderr,"Can't stat %s\n",curdir);
        return;
      }      
    
      /*format information*/
      mode = sbuf.st_mode;
      if( S_ISLNK( mode ) ) type = 'l'; /*symlink*/
      else if( S_ISDIR ( mode ) ) type = 'd'; /*directory*/
      else if( S_ISCHR ( mode ) ) type = 'c'; /*character raw device*/
      else if( S_ISBLK ( mode ) ) type = 'b'; /*block raw device*/
      else if( S_ISFIFO( mode ) ) type = 'p'; /*named pipe*/
      else if( S_ISSOCK( mode ) ) type = 's'; /*Unix domain socket*/
      else if( S_ISREG ( mode ) ) type = '-'; /*regular file*/
      if( mode & S_IRUSR ) usr[0] = 'r';
      if( mode & S_IWUSR ) usr[1] = 'w';
      if( mode & S_IXUSR ) usr[2] = 'x';
      if( mode & S_ISUID ) usr[2] = 's'; /* set UID bit*/
      if( mode & S_IRGRP ) grp[0] = 'r';
      if( mode & S_IWGRP ) grp[1] = 'w';
      if( mode & S_IXGRP ) grp[2] = 'x';
      if( mode & S_ISGID ) grp[2] = 's'; /* set GID bit*/
      if( mode & S_IROTH ) oth[0] = 'r';
      if( mode & S_IWOTH ) oth[1] = 'w';
      if( mode & S_IXOTH ) oth[2] = 'x';
    
      my_passwd = getpwuid(sbuf.st_uid);
      if(my_passwd)
        strncpy(uid,my_passwd->pw_name,8);
      else
        snprintf(uid,9,"%d",sbuf.st_uid);
    
      my_group = getgrgid(sbuf.st_gid);
      if(my_group)
        strncpy(gid,my_group->gr_name,8);
      else
        snprintf(gid,9,"%d",sbuf.st_gid);
    
      time(&now);
      year = localtime(&now)->tm_year;
      strtime = localtime(&sbuf.st_ctime);
      if(strtime->tm_year == year)
        strftime(mod,13,"%b %e %R",strtime);
      else
        strftime(mod,13,"%b %e %Y",strtime);
             
      /*print the details*/
      printf("%c%s%s%s    %d %s     %s         %d\t%s %s\n",
             type,usr,grp,oth,sbuf.st_nlink,
             uid,gid,sbuf.st_size,mod,filename);
      free(curdir);
      /*free(path);*/
    }
    
    int main(int argc, char **argv) {
      int count,index;
      char *curdir;
      struct stat sbuf;
      struct dirent **direntp;
    
      curdir = workdir(); 
    
      if(argv[1] != '\0'){
        if(stat(argv[1], &sbuf) != -1){
          if(S_ISDIR(sbuf.st_mode)){ /* Is a directory */
            count = scandir(argv[1],&direntp,NULL,alphasort);
            if(count){
              for(index=0; index<count; ++index){
                printDetails(direntp[index]->d_name,argv[1]);
                free(direntp[index]);
              }
            }else{
              fprintf(stderr,"Can't open %s\n",argv[1]);
              exit(EXIT_FAILURE);
            }
          }else if(S_ISREG(sbuf.st_mode)){ /* Is a regular file */
            printDetails(argv[1],NULL);
          }else if(S_ISLNK(sbuf.st_mode))
            printDetails(argv[1],NULL);               
        }else
          printf("Can't stat %s\n", argv[1]);
      }else{
        printf("%s\n",curdir);
        if(stat(curdir,&sbuf) !=-1){
          count = scandir(curdir,&direntp,NULL,alphasort);
          if(count){
            for(index=0; index<count; ++index){
              printDetails(direntp[index]->d_name,NULL);
              free(direntp[index]);
            }
          }else{
            fprintf(stderr,"Can't open %s\n",curdir);
            exit(EXIT_FAILURE);
          }
        }else
          fprintf(stderr,"Can't stat %s\n",curdir);
      }
      free(curdir);
      free(direntp);
      return 0;
    }
    I thought about trying

    Code:
    free(path);
    But this doesn't seem to work.
    PuterPaul.co.uk - Portfolio site

  2. #2
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    Ok, I'll give you three guesses to tell me what is wrong with this chain of events:

    1) void printDetails(char *filename,char *path){
    ... the prototype

    2) printDetails(argv[1],NULL);
    ... the call

    3) strcat(path,"/");
    ... the strcat


    Gee. Any idea why that fails?
    Additionally, you can't free a null pointer. Well you CAN...

    Quzah.
    Hope is the first step on the road to disappointment.

  3. #3
    Registered User
    Join Date
    Mar 2002
    Posts
    87
    I'd agree with that diagnosis, except that printDetails only gets executed when the command line argument it encounters is either a link or a regular file.

    Code:
    }else if(S_ISREG(sbuf.st_mode)){ /* Is a regular file */
            printDetails(argv[1],NULL);
    }else if(S_ISLNK(sbuf.st_mode))
            printDetails(argv[1],NULL);
    What i'm talking about is when the command line encounters a directory.

    Code:
    if(S_ISDIR(sbuf.st_mode)){ /* Is a directory */
            count = scandir(argv[1],&direntp,NULL,alphasort);
            if(count){
              for(index=0; index<count; ++index){
                printDetails(direntp[index]->d_name,argv[1]);
                free(direntp[index]);
              }
            }else{
              fprintf(stderr,"Can't open %s\n",argv[1]);
              exit(EXIT_FAILURE);
            }
    }
    And besides the concatenation code should only be reached if the following code fails.

    Code:
      if(stat(curdir,&sbuf) != -1){
        if(S_ISLNK(sbuf.st_mode)){
          if(readlink(curdir,buf,bufsize) != -1){
            if(lstat(buf,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
          }else
            fprintf(stderr,"Can't readlink %s\n",filename);
        }else if(S_ISREG(sbuf.st_mode)){
          if((fdes = open(curdir,O_RDONLY)) != ENOENT){
            if(fstat(fdes,&sbuf) == -1){
              fprintf(stderr,"Can't stat %s\n",filename);
              return;
            }
            close(fdes);
          }else{
            fprintf(stderr,"Can't open %s\n",filename);
            return;
          }
        }
      }
    PuterPaul.co.uk - Portfolio site

  4. #4
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    My point is still relevant. Additionally, you cannot simply concat onto the end of the argv pointers. This is like trying to:

    char *s = "Hello. Concatting here is a BadThing(TM)";

    strcan( s, "This is wrong to do." );

    While your compiler MAY let you do it, it is WRONG to do so. Command line arguments, if I recall correctly, are just like string literals. They are not to be modified. This is your problem.

    Besides...

    And besides the concatenation code should only be reached if the following code fails.
    Even if it "should" only ever bee reached in some rare occasion, my point is still valid:

    THIS WILL CRASH YOUR PROGRAM:
    }else if(S_ISREG(sbuf.st_mode)){ /* Is a regular file */
    printDetails(argv[1],NULL);
    }else if(S_ISLNK(sbuf.st_mode))
    printDetails(argv[1],NULL);
    }else
    printf("Can't stat %s\n", argv[1]);
    }else{
    Concating onto a NULL pointer WILL crash your program. As such, you should fix this, because it is potentially erronious code.

    And like I said: You cannot concat onto string literals.

    Quzah.
    Hope is the first step on the road to disappointment.

  5. #5
    Registered User
    Join Date
    Mar 2002
    Posts
    87
    char *s = "Hello. Concatting here is a BadThing(TM)";

    strcan( s, "This is wrong to do." );
    Why is it wrong tho??

  6. #6
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,897
    >Why is it wrong tho??
    You cannot modify a string literal.

    -Prelude
    My best code is written with the delete key.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Correct usage of malloc and free for 2D array
    By disruptivetech in forum C Programming
    Replies: 1
    Last Post: 10-20-2008, 05:20 AM
  2. Replies: 12
    Last Post: 06-24-2005, 04:27 PM
  3. Help needed with backtracking
    By sjalesho in forum C Programming
    Replies: 1
    Last Post: 11-09-2003, 06:28 PM
  4. "if you love someone" :D
    By Carlos in forum A Brief History of Cprogramming.com
    Replies: 12
    Last Post: 10-02-2003, 01:10 AM