Thread: [C] Pipe that hangs

Hi!

I'm writing a little shell that behaves like old Thompson shell. I've a big problem with the creation of pipe mechanism. I must create pipe mechanism like UNIX shell (cmd1 | cmd2 | ... | cmdn). Below there is a portion of my function:

Code:

int j=0;
for(i=0; i<res; ++i){
      if((pid = fork()) < 0){
         perror("fork error");
         exit(FORK_ERR);
      }
      if(pid == 0){  
         if(args!=NULL){
            free(args);
            args=NULL;
         } 
         args=(char **)calloc(MAX_ARGS,sizeof(char *));
         if(do_I_launch_parse(commands[i]) == 1)
            parse(commands[i],args);
         else{
            if(args[j]==NULL)
               args[j]=(char *)calloc(strlen(commands[i])+1,sizeof(char));
            args[j]=strtok(commands[i]," ");
            ++j;
            while((s=strtok(NULL," ")) != NULL){
               if(args[j]==NULL)
                  args[j]=(char *)calloc(strlen(commands[i])+1,sizeof(char));  
               strcpy(args[j],s);
               ++j;
            }
         }
         if(i==0){
            close(p1[0]);
            close(p2[1]);       
            close(p2[0]);
            if(dup2(p1[1],STDOUT_FILENO) == -1){
               perror("dup error");
               exit(DUP_ERR);
            }
            //close(p2[1]);
         }
         else if(i==res-1){
            if(i%2==0){
               close(p2[1]);
               close(p1[0]);
               close(p1[1]);
               if(dup2(p2[0],STDIN_FILENO) == -1){
                  perror("dup error");
                  exit(DUP_ERR);
               }
              // close(p2[0]);
            }
            else{
               close(p1[1]);
               close(p2[0]);
               close(p2[1]);
               if(dup2(p1[0],STDIN_FILENO) == -1){
                  perror("dup error");
                  exit(DUP_ERR);
               }
               //close(p1[0]);
            }
         } 
         else if(i%2==0){
            close(p2[1]);
            close(p1[0]);
            if(dup2(p2[0],STDIN_FILENO) == -1){
               perror("dup error");
               exit(DUP_ERR);
            }       
            //close(p2[0]);
            if(dup2(p1[1],STDOUT_FILENO) == -1){
               perror("dup error");
               exit(DUP_ERR);
            }       
            //close(p1[1]);
         }
         else{
            close(p2[0]);
            close(p1[1]);
            if(dup2(p1[0],STDIN_FILENO) == -1){
               perror("dup error");
               exit(DUP_ERR);
            }       
            //close(p1[0]);
            if(dup2(p2[1],STDOUT_FILENO) == -1){
               perror("dup error");
               exit(DUP_ERR);
            } 
            //close(p2[1]); 
         }
         ((args[0]==NULL) ? execlp(commands[i],commands[i],NULL) : execvp(commands[i],args));
         perror("exec failed");
         exit(EXEC_ERR); 
      }
      if(pid > 0){
         if (waitpid(pid,&status,0) == -1){
            if(errno!=EINTR){
               perror("waitpid error");
               exit(WAIT_ERR);
            }
            else{
               kill(pid,SIGTERM);
               do{
                  if (waitpid(pid,&status,0) != -1)
                     break;
               }while(errno==EINTR);
            }
         }
      }
   }

Ignore all the strings management. The problem (I hope) is in the pipe. If i launch a command like "ls -l | grep *.h", I receive this output:

Code:

/home/manugal/Progetto  $ ls -l | grep *.h
-rw-r--r-- 1 manugal manugal  3364 2007-01-11 13:28 functions.h
-rw-r--r-- 1 manugal manugal  3371 2007-01-11 13:10 functions.h~

and after the output of second command, it hangs and it returns at prompt only if I press CTRL-C (it seems like that wait any data on pipe), but I don't understand why.

In fact, in the code, if I add a line that set p[0] (that reads) in O_NONBLOCK mode, the output is:

Code:

/home/manugal/Progetto  $ ls -l | grep *.h
-rw-r--r-- 1 manugal manugal  3364 2007-01-11 13:28 functions.h
-rw-r--r-- 1 manugal manugal  3371 2007-01-11 13:10 functions.h~
grep: (standard input): Resource temporarily not available

Why? I've tried also to create two pipes to resolve the problem but nothing. Please, help me

Thanks

Erm, there's plenty of basics to clear up first

> if(args!=NULL)
> free(args);
Except following the fork(), there are now two copies of the allocated memory, and you're only releasing one of them. It's a memory leak in the parent.

> args=(char **)calloc(MAX_ARGS,sizeof(char *));
1. Don't cast the return result of malloc / calloc etc (see the FAQ
2. All-bits zero implied by calloc doesn't necessarily mean it is a valid and NULL pointer - see http://c-faq.com/malloc/calloc.html

> args[j]=(char *)calloc(strlen(commands[i])+1,sizeof(char));
> args[j]=strtok(commands[i]," ");
Another memory leak. strtok() returns a pointer to the innards of your commands string, and overwrites part of that string. You certainly can't then do free( args[j] ) later on.

Some of your strlen() calls are highly dubious as well, given that strtok() is modifying the same string as well.

> close(p2[1]);
> close(p2[0]);
Closing both ends of the same pipe would seem to make it useless as a pipe.
Also, use some constants to identify the READ end and the WRITE end of the pipe.
Also, where are all these pipes being created?

I don't understand why you have 5 separate cases for closing / dup'ing various ends of various pipes. I can see why you might need 3 (for start, middle and end) of the whole pipeline.

Thanks for the reply

Ok. It's obvious that you say these things to me. I post only the code part that interesting this functionality. Naturally It's not the whole program. This is only a for cycle of a function that it would have to execute pipe. Let's start:

It's a memory leak in the parent.

No, because when the function terminate, it return to main function that free all dinamically allocated variables. That control is there, because args sometimes has dirty values.

args=(char **)calloc(MAX_ARGS,sizeof(char *));

Perhaps here you have reason , but I am accustomed to cast when i call calloc or malloc.

Another memory leak. strtok() returns a pointer to the innards of your commands string, and overwrites part of that string. You certainly can't then do free( args[j] ) later on.

When I free memory on a pointer to pointer I call free() function in this manner:

Code:

if(args!=NULL){
   free(args);
   args=NULL;
}

but I don't call free for every string at which point args. It's wrong?

Some of your strlen() calls are highly dubious as well, given that strtok() is modifying the same string as well.

Because I've not post the whole program. I use all these strings management to subdivide arguments. For example if I have "ls -l *.c" I divide this string in "ls" "-l" "*.c". Then there are other problem, for example "*.c" what does mean? Therefore I must to call parse function that use wordexp to expand "*.c" etc. etc. . It's quite complex. However, to the end, the string passed to exec function is correct (in fact the command is executed correctly).

Closing both ends of the same pipe would seem to make it useless as a pipe.

But I have only closed the ends of pipe that I not using in that part of code.

Finally, I've 5 separate cases because is the only method that it has come in my mind


I hope that it's all clear.

Originally Posted by Manugal

When I free memory on a pointer to pointer I call free() function in this manner:

Code:

if(args!=NULL){
   free(args);
   args=NULL;
}

but I don't call free for every string at which point args. It's wrong?

yes, it is wrong.
The basic rule is if you got some pointer from calloc/malloc - you should call free for this pointer (I'm not considering now the realloc case...)

so if you called malloc to initialize args[i] - you save this pointer till the memory is of no use anymore and call free for it.
if you modifying the pointer and loosing the original value - you got memory leak.
if you store the pointer in the dynamic array of pointers and then free the memory where array is stored without freeing each pointer beforehead - it is a memory leak too.

How about you stop closing all of those pipes, and try using them for something like reading / writing, and display what you get from them? You know, to debug the issue.

Quzah.

You could always make a simple version that only has a pair of pipes. Make sure you understand how they communicate. Prepend a bit of info from each one to the buffer, send it back and forth as each reads the message. Start small. Get that working. Then try adding one more child to the mix.


Quzah.

Ok. I've tried it. With two programs no problem (I play start the first program in child and the second in parent). With one more command, return the old problem. Now it's the code (res variable is the count of programs in the pipe). It's similarly to that one before, but now I also have the error message that give me gdb:

Code:

int j=0;
....

if((pid=fork()) < 0){
   perror("fork error");
   exit(FORK_ERR);
}
for(i=0; i<res; ++i){
   if(pid == 0){
      if(do_I_launch_parse(commands[i]) == 1)
            parse(commands[i],args);
         else{
            if(args[j]==NULL)
               args[j]=(char *)calloc(strlen(commands[i])+1,sizeof(char));
            args[j]=strtok(commands[i]," ");
            ++j;
            while((s=strtok(NULL," ")) != NULL){
               if(args[j]==NULL)
                  args[j]=(char *)calloc(strlen(commands[i])+1,sizeof(char));  
               strcpy(args[j],s);
               ++j;
            }
         }
         if(i==0){
            close(p[0]);
            dup2(p[1],STDOUT_FILENO);
            close(p[1]);
         }
         else if(i==res-1){
            close(p[1]);
            dup2(p[0],STDIN_FILENO);
            close(p[0]);
         }
         else{
            dup2(p[0],STDIN_FILENO);
            dup2(p[1],STDOUT_FILENO);
         }
         if((pid1=fork())==0){ /* second child */
            ((args[0]==NULL) ? execlp(commands[i],commands[i],NULL) : execvp(commands[i],args));
            perror("exec failed");
            exit(EXEC_ERR);
         }
         else if(pid1 < 0){
            perror("fork error");
            exit(FORK_ERR);
         }
         if (waitpid(pid1,&status,0) == -1){
            if(errno!=EINTR){
               perror("waitpid error");
               exit(WAIT_ERR);
            }
            else{
               kill(pid1,SIGTERM);
               do{
                  if (waitpid(pid1,&status,0) != -1)
                     break;
               }while(errno==EINTR);
            }
         }
      }
      }
      if(pid > 0){
         if (waitpid(pid,&status,0) == -1){
            if(errno!=EINTR){
               perror("waitpid error");
               exit(WAIT_ERR);
            }
            else{
               kill(pid,SIGTERM);
               do{
                  if (waitpid(pid,&status,0) != -1)
                     break;
               }while(errno==EINTR);
            }
         }
       }

With gdb when I arrive in the second child it (when I on the second command), it would execute the second command, but it no return any output and when it calls the wait in parent (the first child), it give to me this message two times:

Program received signal SIGTTIN, Stopped (tty input).

Certainly, it returned this message also with the previous code that I post (but in that case in parent directly). Why it gives me this message?

P.S.: I had not using any buffer, because I don't know how and in which point capture data.