when calloc/malloc are vital , when could be omited.

in codes bellow , since allocated some memory for d (and never was free) , i want to add free (d) inorder to fix usages of resources in that application. , i had tried servral parts of the code , to free it, but the result was , Segment-fault or glibc detected invalid free or undesired result (like black screen).

the author of the codes , commented (FIXME) and said , this memory should be free after (read_from_pipe) , i already had tried it , but the result is segfault in other part of the code (which seems adding free prevent them to access this data so segfault

Code:

int32_t write_to_pipe(int32_t fd, int32_t id, uchar *data, int32_t n)
{
    if (!fd) {
        cs_log("write_to_pipe: fd==0 id: %d", id);
        return -1;
    }

    //cs_debug_mask(D_TRACE, "write to pipe %d (%s) thread: %8X to %8X", fd, PIP_ID_TXT[id], pthread_self(), get_thread_by_pipefd(fd)->thread);

    uchar buf[3+sizeof(void*)];

    // fixme
    // copy data to allocated memory
    // needed for compatibility
    // need to be freed after read_from_pipe

    void *d;
    if(!cs_malloc(&d, n, -1)) return -1;
    memcpy(d, data, n);

    if ((id<0) || (id>PIP_ID_MAX))
        return(PIP_ID_ERR);

    memcpy(buf, PIP_ID_TXT[id], 3);
    memcpy(buf+3, &d, sizeof(void*));

    n=3+sizeof(void*);

    return(write(fd, buf, n));
}

above is the part of the code (write_to_pipe) : (if need to entire source codes , here is the link :

oscam.c in trunk



My question is :

1. surely need to free d (which already allocated by cs_malloc) , author of the source in his FIXME comment's believe it should be somewhere after read_pipe , i had tried , got segfault. does any expert could guide me in which function (line) i should free this allocated memory which also not break application from working?


2. @expert , I had just thought maybe it's not required to allocated memory for d , (i mean maybe i should omit allocation , the author in FIXme comment said need for compabality , but i am not sure what he means ) and just memcpy data to buf . am i in correct point? (actually i had tried to remove that cs_malloc , but the result was segfault in other part of code...

Thanks in advance

Code:

    memcpy(buf+3, &d, sizeof(void*));
 
    n=3+sizeof(void*);
 
    return(write(fd, buf, n));

This makes no sense at all.
You're writing a POINTER down a pipe to some other process.
That address isn't going to mean anything at the other end.

The only way this is going to work is if both the writer and reader are in the same process (but possibly different threads, as some of the comments allude to).

Without seeing the read_pipe code (and knowing that it really is in the same process - can you prove that), I'm saying no more.

TBH, it's a lot easier to write (and explain) if you do this.

Code:

    write(fd, PIP_ID_TXT[id], 3);  // the magic cookie
    write(fd,&n,sizeof(n));  // the length of the buffer (in n)
    write(fd,data,n); // n bytes of data

The reader code does these things
- reads the 3 byte cookie
- reads sizeof(int32_t) bytes to read the length (then allocates that much)
- reads 'length' bytes of data into the freshly allocated buffer.


But then again, we've been kicking this can around for a while now.
mtrace : Free was never allocated
malloc , how could free a return value?

@Dear Salem

I am not authour of the source code , but i know (u are right) , the code had been written very complicated and buggy.

Well , i am not sure if i got your last post meaning completely , did u mean , i should also change function write_to_pipe to Void?

this is what i had got from your last post , i made that function to void , and also i replace your 3 lines of code , instead of line 17 to 29 but after this change i got segfault about reader_do_pipe function in oscam-reader.c , when i backtrace i saw it return reader as 0x000000;

Code:

write(fd, PIP_ID_TXT[id], 3);  // the magic cookie
write(fd,&n,sizeof(n));  // the length of the buffer (in n)
write(fd,data,n);

is this what u exactly meant in your last post (to change the function to void (from int32_t) and replace those 3 lines of code , instead of line 17 to 29 (line refers to the first post of this thread)?



here is rea_from_pipe function if it would be needed :

Code:

 int32_t read_from_pipe(struct s_client *client, uchar **data) { 
   if(!client)          return PIP_ID_ERR;     
 int32_t n, rc, fd = client->fd_m2c_c;     
 intptr_t hdr=0;    
 uchar buf[3+sizeof(void*)]; 
    memset(buf, 0, sizeof(buf));

    *data=(uchar *)0;
     rc=PIP_ID_NUL;   
  cs_writelock(&client->pipelock); 
     n=read(fd, buf, sizeof(buf));  
    if(client->pipecnt >= n) client->pipecnt -= n;     
 cs_writeunlock(&client->pipelock);   
   if (n==sizeof(buf)) {         memcpy(&hdr, buf+3, sizeof(void*));  
   } else {         cs_log("WARNING: pipe header to small !");  
       return PIP_ID_ERR;    
 }         //uchar id[4];     //memcpy(id, buf, 3);     //id[3]='\0';     //cs_debug_mask(D_TRACE, "read from pipe %d (%s) thread: %8X", fd, id, (uint32_t)pthread_self());
     int32_t l;  
   for (l=0; (rc<0) && (PIP_ID_TXT[l]); l++)      
   if (!memcmp(buf, PIP_ID_TXT[l], 3))    
         rc=l;    
 if (rc<0) {    
     cs_log("WARNING: pipe garbage from pipe %i", fd);   
      return PIP_ID_ERR;     }   
  *data = (void*)hdr;    
 return(rc); }

P.S : although the code was written very bugy , but i had fixed so many null pointer , endless loop , fix so many memery leaks issues , and made this application works much better from last year so far (i really always appreciate your helpful comments , u are always helpful for this programming comuinity , with your great comment and ideas.)

Best Regards

Jeez, I hope that isn't as badly formatted in your source editor as it is posted here.

Code:

int32_t read_from_pipe(struct s_client *client, uchar ** data)
{
  if (!client)
    return PIP_ID_ERR;

  int32_t n, rc, fd = client->fd_m2c_c;
  intptr_t hdr = 0;
  uchar buf[3 + sizeof(void *)];
  memset(buf, 0, sizeof(buf));

  *data = (uchar *) 0;
  rc = PIP_ID_NUL;

  cs_writelock(&client->pipelock);
  n = read(fd, buf, sizeof(buf));
  if (client->pipecnt >= n)
    client->pipecnt -= n;
  cs_writeunlock(&client->pipelock);

  if (n == sizeof(buf)) {
    memcpy(&hdr, buf + 3, sizeof(void *));
  } else {
    cs_log("WARNING: pipe header to small !");
    return PIP_ID_ERR;
  }
  //uchar id[4];     
  //memcpy(id, buf, 3);     
  //id[3]='\0';     
  //cs_debug_mask(D_TRACE, "read from pipe %d (%s) thread: %8X", fd, id, (uint32_t)pthread_self());
  int32_t l;
  for (l = 0; (rc < 0) && (PIP_ID_TXT[l]); l++)
    if (!memcmp(buf, PIP_ID_TXT[l], 3))
      rc = l;

  if (rc < 0) {
    cs_log("WARNING: pipe garbage from pipe %i", fd);
    return PIP_ID_ERR;
  }

  *data = (void *) hdr;
  return (rc);
}

Anyway, for the moment, I would leave everything alone until you understand what everything is for.

It we assume that everything is running in the same address space, then the correct place to free the memory would be here.

Code:

uchar *p;  // the received memory pointer
struct s_client c;  // the client
int32_t result = read_from_pipe(&c, &p);
// do stuff with p
free( p );

This you need to do in every caller to read_from_pipe() when they've finished with the memory buffer obtained through reading the pipe.

It we assume that everything is running in the same address space, then the correct place to free the memory would be here.
This you need to do in every caller to read_from_pipe() when they've finished with the memory buffer obtained through reading the pipe.

right , but i knew this ( i and the author of the code had considered those free after every caller to read_from_pipe) , but my question (in first post of this thread) refers to how correct write_to_pipe memory allocation (cs_malloc , and those none-sence memcpy) ?

Code:

write(fd, PIP_ID_TXT[id], 3);  // the magic cookie

write(fd,&n,sizeof(n));  // the length of the buffer (in n)

write(fd,data,n);

i really want to correct it as u had offered (the way u think it should be changed) , i tried it in different ways but the app not start-up , still i am eager to correct it. in my first attempt as said , i did exactly as you had suggested , the result was i got a segfault in oscam-reader.c:reader_do_pipe . in my next (last attempt) i had changed a bit your 3 lines (i changed back buf) , i mean i made write_to_pipe like this :

Code:

int32_t write_to_pipe(int32_t fd, int32_t id, uchar *data, int32_t n)
{     
    if ((id<0) || (id>PIP_ID_MAX))
        return(PIP_ID_ERR);

    //cs_debug_mask(D_TRACE, "write to pipe %d (%s) thread: %8X to %8X", fd, PIP_ID_TXT[id], pthread_self(), get_thread_by_pipefd(fd)->thread);
  /*  
    // fixme
    // copy data to allocated memory
    // needed for compatibility
    // need to be freed after read_from_pipe
    void *d;
    if((d = cs_malloc(n)) == NULL)    return -1;

    memcpy(d, data, n);
    memcpy(buf, PIP_ID_TXT[id], 3);
    memcpy(buf+3, &d, sizeof(void*));

    n=3+sizeof(void*);

    return(write(fd, buf, n));
*/

    uchar buf[3+sizeof(void*)];

write(*buf, PIP_ID_TXT[id], 3);  // the magic cookie
write(*(buf+3),data,sizeof(void*));  // the length of the buffer (in n)

n=3+sizeof(void*);

return(write(fd,buf,n)); // n bytes of data

but when i run , i got this :

*** glibc detected *** /var/bin/oscam: munmap_chunk(): invalid pointer: 0x297a063e ***
*** glibc detected *** /var/bin/oscam: munmap_chunk(): invalid pointer: 0x297a063e ***

i know that the change i had made myself is not correct ,but i have no idea , how i could fix that none-sence part of the code (which the author of the code had written in write_to_pipe) , that's why i asked here from experts (like u)

P.S : here is the entire source codes if it's needed.


Thanks in advance

This is write (without error checks, and other data processing)

Code:

int32_t write_to_pipe(int32_t fd, int32_t id, uchar *data, int32_t n)
{
    write(fd, PIP_ID_TXT[id], 3);  // the magic cookie
    write(fd,&n,sizeof(n));  // the length of the buffer (in n)
    write(fd,data,n); // n bytes of data
}

This is read (without error checks, and other data processing)

Code:

int32_t read_from_pipe(struct s_client *client, uchar ** data)
{
  uchar buf[3];
  int32_t n;

  read(fd, buf, sizeof(buf));
  read(fd, &n, sizeof(n));
  *data = malloc(n);  //!! caller calls free()
  read(fd, *data, n);
}

It really doesn't matter what you write and read, so long as both ends MATCH.

If you start hacking away at one end, then you have to make sure the change is reflected at the other end as well.

How big is n anyway?
Perhaps if n is large (say 10's of Kbytes or more), being sent many times a second, then the approach you have (passing a pointer) is perhaps OK if it's basically working (except for the odd leak or two).

@Dear Salem

I did as u suggested , now the good news is the program did not get segfault (maybe the reason is i had changed read_from_pipe and add those 3 line of read and also add malloc (data) in read_from_pipe which was not in origianl code as u had suggested in your last comment) but (the regression is) after this change i got black screen (which with those original (none-sence) of codes , black screen not happens)

How big is n anyway?

n = 452 , the sizeof (n) is 4


P.S :

this is how i did tried in my last attempt (i put last write , in return)

Code:

int32_t write_to_pipe(int32_t fd, int32_t id, uchar *data, int32_t n)
{
    
    if ((id<0) || (id>PIP_ID_MAX))
        return(PIP_ID_ERR);

   write(fd, PIP_ID_TXT[id], 3);  // the magic cookie
    write(fd,&n,sizeof(n));  // the length of the buffer (in n)
   return( write(fd,data,n)); // n bytes of data
    
}

Code:

/*
 * read_from_pipe():
 * read all kind of data from pipe specified by fd
 * special-flag redir: if set AND data is ECM: this will redirected to appr. client
 */
int32_t read_from_pipe(int32_t fd, uchar **data)
{
    int32_t rc;
    intptr_t hdr=0;
    uchar buf[3];  
    memset(buf, 0, sizeof(buf));

    *data=(uchar *)0;
    rc=PIP_ID_NUL;
  int32_t n;
 
  read(fd, buf, sizeof(buf));
  read(fd, &n, sizeof(n));
 *data = malloc(n); //!! caller calls free()
  read(fd, *data, n);


/*
    if (read(fd, buf, sizeof(buf))==sizeof(buf)) {
        memcpy(&hdr, buf+3, sizeof(void*));
    } else {
        cs_log("WARNING: pipe header to small !");
        return PIP_ID_ERR;
    }
*/

    int32_t l;
    for (l=0; (rc<0) && (PIP_ID_TXT[l]); l++)
        if (!memcmp(buf, PIP_ID_TXT[l], 3))
            rc=l;

    if (rc<0) {
        cs_log("WARNING: pipe garbage from pipe %i", fd);
        return PIP_ID_ERR;
    }

    *data = (void*)hdr;

    return(rc);
}

i would like also to point some functions which used write_to_pipe , and read_pipe , maybe after this change , i need some changes in other functions too .

Code:

static int32_t write_ecm_request(int32_t fd, ECM_REQUEST *er)
{
  return(write_to_pipe(fd, PIP_ID_ECM, (uchar *) er, sizeof(ECM_REQUEST)));
}

Code:

int32_t process_client_pipe(struct s_client *cl, uchar *buf, int32_t l) {
    if (!cl) return -1;
    uchar *ptr;
    uint16_t n;
    int32_t pipeCmd = read_from_pipe(cl->fd_m2c_c, &ptr);

    switch(pipeCmd) {
        case PIP_ID_ECM:
            chk_dcw(cl, (ECM_REQUEST *)ptr);
            break;
        case PIP_ID_UDP:
            if (ptr[0]!='U') {
                cs_log("INTERNAL PIPE-ERROR");
            }
            memcpy(&n, ptr+1, 2);
            if (n+3<=l) {
                memcpy(buf, ptr, n+3);
            }
            break;
        case PIP_ID_ERR:
            cs_exit(1);
            break;
        default:
            cs_log("unhandled pipe message %d (client %s)", pipeCmd, cl->account->usr);
            break;
    }
    if (ptr) free(ptr);
    return pipeCmd;
}

highly appreciated if need any other corection to fix it (and make it run) .
Best Regards.

Now line 42 *data = (void*)hdr; is just plain wrong.
You allocate locally, and then read into the buffer

hdr is the relic from the old way.

Originally Posted by Salem

Now line 42 *data = (void*)hdr; is just plain wrong.

@salem
bingo , u are brilliant (and very inteligent) , now it works perfectly as it should, and that wrong (none-sence) memory allocation in write_to_pipe is fixed by u.


another important issue about resource usage of this app (which obviously could be optimized and cause huge improvement ) is in module-datastruct-list.c. may i share these part of code here , and if u would feel any change , or refactoring of them is needed , would appreciate to tell. The fact is , functions ll_create() , ll_appened , ll_appened_nolocks , ll_prepend is being used in many parts of this app's source code , in each of those function memory is allocated and since it return that memory allocated variable , i should free them after calling them , but when after calling of these functions , i want to free those allocated memory , sometimes it casue , the app black screen , therefore , do u belive these 4 functions could be rewritten in better way?

Code:

LLIST *ll_create() 
{     
LLIST *l = cs_malloc(&l, sizeof(LLIST), 0);   
  cs_lock_create(&l->lock, 5, "ll_lock");
     return l; 
}

Code:

 LL_NODE* ll_append_nolock(LLIST *l, void *obj) {   
  if (l && obj){        
 LL_NODE *new;   
      if(!cs_malloc(&new,sizeof(LL_NODE), -1)) return NULL; 
        new->obj = obj;   
          if (l->last)             l->last->nxt = new;      
   else             l->initial = new;       
  l->last = new;            
          l->count++;      
   return new;    
 }      
return NULL; 
}

Code:

 LL_NODE* ll_append(LLIST *l, void *obj) {     if (l && obj) {        
 if (!ll_lock(l)) return NULL; 
    LL_NODE *n = ll_append_nolock(l, obj);    
     ll_unlock(l);   
      return n;   
  }  
   return NULL; 
}

Code:

LL_NODE *ll_prepend(LLIST *l, void *obj) {   
  if (l && obj) {         
               LL_NODE *new;    
     if(!cs_malloc(&new,sizeof(LL_NODE), -1)) return NULL;     
    if (!ll_lock(l)) { add_garbage(new); return NULL;
 }    
        new->obj = obj;  
       new->nxt = l->initial;        
  l->initial = new;    
     if (!l->last)             l->last = l->initial;   
      l->count++;    
     ll_unlock(l);   
       return new;    
 } 
     return NULL; 
}

These 4 above functions are very important and effective in memory usages of this program and as some analysis tools shows , the memory of allocated for them not free (and cause some memory leaks) , therefore if u could see anything wrong about above code , please tell me.

P.S : here is the entire source code for module-datastruct-list.c

P.S.2: ie i would free allcoated memory for ll_create call like this

Code:

LLIST *u = ll_create()
card->providers  = u;
//// do stuff with u and card->providers

free (u);

just as a question :

this way i free after each ll_create call sometimes cause black screen and undesired results , maybe instead of just calling free (like above) i shall write some other functions in module-datastruct-list.c like ( ie for ll_create i should use ll_destroy or ll_clear) to free those allocated memory . and for free ll_append and ll_prepend allocation , i need to write new functions , then i could call those new functions to free? if the answer yes , and i have to add new functions for free ll_prepend and ll_appened , how the new functions shall be? ( or just is needed to rewrite (correct) ll_appened and ll_prepend? )

Tons of Thanks.

@Salem
Regard the change read_from_pipe u said in this thread , it works fien when start running the app , but after 10-15 mins gets segfault (in below except backtrace , i write which line of that function cause this segfault , since bt not show anyting especial i also write here app log which indicate what is the last lines running just before segfault but it's a bit complicated for me to find why this segfault happens , any idea is appreciated,

2C8FF4E8 c unhandled pipe message 1361618757 (client )
2C8FF4E8 c WARNING: pipe garbage from pipe 23

2C8FF4E8 c thread 2C8FF4E8 exit with signal 1
2C8FF4E8 c thread 2C8FF4E8 ended!
[New LWP 1639]
2A0E94E8 p notice: changed dcw checksum byte cw[15] from 78 to 88
warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available.
*** glibc detected *** /var/bin/oscam: munmap_chunk(): invalid pointer: 0x297b6038 ***

Program received signal SIGABRT, Aborted.
[Switching to LWP 1639]
0x297e1678 in raise () from /lib/libc.so.6
(gdb) bt
#0 0x297e1678 in raise () from /lib/libc.so.6
#1 0x297e2a4c in abort () from /lib/libc.so.6
Backtrace stopped: frame did not save the PC

line 27, refers to error log qouted above just right before segfault

Code:

int32_t read_from_pipe(int32_t fd, uchar **data) {
     int32_t rc;
     intptr_t hdr=0; 
    uchar buf[3];
       memset(buf, 0, sizeof(buf));
      *data=(uchar *)0;  
   rc=PIP_ID_NUL;   
int32_t n;    
 read(fd, buf, sizeof(buf));
   read(fd, &n, sizeof(n)); 
 *data = malloc(n); //!! caller calls free()  
 read(fd, *data, n);
   /*     
if (read(fd, buf, sizeof(buf))==sizeof(buf)) 
{    
     memcpy(&hdr, buf+3, sizeof(void*));  
   } else {    
     cs_log("WARNING: pipe header to small !"); 
        return PIP_ID_ERR;     
} */      

int32_t l;   
  for (l=0; (rc<0) && (PIP_ID_TXT[l]); l++)    
     if (!memcmp(buf, PIP_ID_TXT[l], 3))      
       rc=l;      
if (rc<0) {       
  cs_log("WARNING: pipe garbage from pipe %i", fd);   
      return PIP_ID_ERR;    
 }      

      return(rc);
 
}

Code:

int32_t process_client_pipe(struct s_client *cl, uchar *buf, int32_t l) {  
   uchar *ptr;    
 uint16_t n;
     int32_t pipeCmd = read_from_pipe(cl, &ptr);     
 
switch(pipeCmd) {      
   case PIP_ID_ECM:         
    chk_dcw(cl, (ECM_REQUEST *)ptr);   
          break;  
       case PIP_ID_UDP:    
         if (ptr[0]!='U') {      
           cs_log("INTERNAL PIPE-ERROR");    
         }           
  memcpy(&n, ptr+1, 2);      
       if (n+3<=l) {        
         memcpy(buf, ptr, n+3);       
      }            
 break;        
 case PIP_ID_ERR:       
      cs_exit(1);         
    break;       


// line bellow refers to the error log qouted above before segfult

  
default:         
    cs_log("unhandled pipe message %d (client %s)", pipeCmd, cl->account->usr);   
          break;    
 }  
   if (ptr) free(ptr);  

   return pipeCmd; 
}

> Backtrace stopped: frame did not save the PC
This usually means one of two things.

1. The code is compiled with this flag

-fomit-frame-pointer
Don't keep the frame pointer in a register for functions that don't need one. This avoids the instructions to save, set up and restore frame pointers; it also
makes an extra register available in many functions. It also makes debugging impossible on some machines.

On some machines, such as the VAX, this flag has no effect, because the standard calling sequence automatically handles the frame pointer and nothing is saved by
pretending it doesn't exist. The machine-description macro "FRAME_POINTER_REQUIRED" controls whether a target machine supports this flag.

Starting with GCC version 4.6, the default setting (when not optimizing for size) for 32-bit Linux x86 and 32-bit Darwin x86 targets has been changed to
-fomit-frame-pointer. The default can be reverted to -fno-omit-frame-pointer by configuring GCC with the --enable-frame-pointer configure option.

Enabled at levels -O, -O2, -O3, -Os.

2. You've trashed the stack by buffer overruns on local variables.

@Salem

i think , now i know why this segfault is happened but don't know how could solve this one ( if i want to debug myself , i could say , This segfault happens whenever function name "cleanup_thread" is called. This would free (cl) , but in other function like "process_client_pipe" still pass "cl" to function like "chk_dcw". free them cause null pointer of cl , and therefore segfault. I qoute clenup_thread (which also call read_from_pipe) .
Also i got a segfault which confrimed my claimed :

0x00408fea in chk_dcw (cl=0x4db008, er=0x4f4be8) at oscam.c:2407
2407 oscam.c: No such file or directory.
in oscam.c
(gdb) bt
#0 0x00408fea in chk_dcw (cl=0x4db008, er=0x4f4be8) at oscam.c:2407
#1 0x0040b5ce in process_client_pipe (cl=0x4db008, buf=0x0, l=0) at oscam.c:3443
#2 0x0043f926 in dvbapi_main_local (cli=0x4db008) at module-dvbapi.c:1862
#3 0x29799486 in ?? () from /lib/libpthread.so.0

x 0x4db008
0x4db008: 0x00000000

Code:

void chk_dcw(struct s_client *cl, ECM_REQUEST *er)

{

  ECM_REQUEST *ert;

  ert=&cl->ecmtask[er->cpti];

// LINE 2407 which is reffer where segfault happens
  if (ert->rc<E_99) {

at the begining of the function chk_dcw even i had added line bellow ,

Code:

if (!cl) return;

in order to check whenever cl is null or zero , return and prevent go to next lines which cause segfault , but even this won't prevent it , i mean while debugging it pass this line (where as cl is zero)!! and in line 2407 segfault happen
,


here is cleanup_thread code
as in line 80 , cl is free , whereas , as explained above , process_client_pipe , pass cl to chk_dcw , which cause segfault,

Code:

void cleanup_thread(void *var)

{

    struct s_client *cl = var;

    if(cl && !cl->cleaned){ //cleaned=0

        cl->cleaned++; //cleaned=1


#ifdef MODULE_CCCAM

        if(cl->typ == 'c'){
            struct cc_data *cc = cl->cc;
            if (cc) cc->mode = CCCAM_MODE_SHUTDOWN;
        }
#endif

        //kill_thread also removes this client, so here just to get sure client is removed:
        struct s_client *prev, *cl2;
        cs_lock(&clientlist_lock);
        for (prev=first_client, cl2=first_client->next; prev->next != NULL; prev=prev->next, cl2=cl2->next)
            if (cl == cl2)
                break;
        if (cl == cl2)
            prev->next = cl2->next; //remove client from list
        cs_unlock(&clientlist_lock);

        cs_sleepms(500); //just wait a bit that really really nobody is accessing client data

        if(cl->typ == 'c' && ph[cl->ctyp].cleanup)
            ph[cl->ctyp].cleanup(cl);
        else if (cl->reader && cl->reader->ph.cleanup)
            cl->reader->ph.cleanup(cl);
      if(cl->typ == 'c'){
        cs_statistics(cl);
        cl->last_caid = 0xFFFF;
        cl->last_srvid = 0xFFFF;
        cs_statistics(cl);
      }

        if(cl->pfd) nullclose(&cl->pfd); //Closing Network socket
        int32_t rc, fd_m2c = cl->fd_m2c, fd_m2c_c = cl->fd_m2c_c;
        if(fd_m2c_c){
            cl->fd_m2c_c = 0;
            cl->fd_m2c = 0;
            cs_sleepms(5);        // make sure no other thread is currently writing...
            struct pollfd pfd2[1];
            pfd2[0].fd = fd_m2c_c;
            pfd2[0].events = (POLLIN | POLLPRI);
            uchar *ptr;
            while ((rc = poll(pfd2, 1, 5)) != 0){
                if(rc == -1){
                    if(errno == EINTR)
                        continue;
                    else
                        break;
                }
                int32_t pipeCmd = read_from_pipe(fd_m2c_c, &ptr);
                if (ptr) free(ptr);
                if (pipeCmd==PIP_ID_ERR || pipeCmd==PIP_ID_NUL)
                    break;
            }
        }
        if(fd_m2c) close(fd_m2c);    //Closing master write fd
        if(fd_m2c_c) close(fd_m2c_c);    //Closing client read fd

        if (cl->reader) {
            cl->reader->client = NULL;
            cl->reader = NULL;
        }
        
#ifdef MODULE_CCCAM
        free(cl->cc);
#endif

    free(cl->serialdata);
        cl->cleaned++;//cleaned=2
        free(cl);
    
        cleanup_ecmtasks(cl);
        free(cl->emmcache);
        free(cl->req);
    
        cs_cleanlocks();
        

    }

}

Maybe after the change of read_from_pipe , wtire_to_pipe , i have to make some change in other functions like cleanup_thread , isn't it?
Best Regards

> cs_sleepms(500); //just wait a bit that really really nobody is accessing client data
Things like this almost always indicate broken design.
When the dynamic behaviour of the system grows, and more clients become active, random delays like this shift in unexpected ways.

> //kill_thread also removes this client, so here just to get sure client is removed:
What happens if the killed client happens to be the first one on the list?

Code:

        if (cl == cl2)
            prev->next = cl2->next; //remove client from list
        else {
            fprintf(stderr,"List fault\n");
            exit(1);
        }

Well , with old read_from_pipe , write_to_pipe code , Though it was quite none-sence but it worked ,and this segfault not happened but the CRONS about that it had a obvious memory leak . with the change u had suggested , now code is quite reasonable and resource usages is fine , but whenever cleanup_thread is called , cl is being 0 , and then when it calls chk_dcw (from process_client_pipe) , it gets segfault (as describe the whole story in my last post)

Now i have 2 option :
1- use old and original none-sence "read_from_pipe" code and stand that memory leask (which is not good and not my desire)
2- use the code u had suggested which make that function reasonable , but still get segfault (whenever cleanup_thread is called. but still i am hopeful , maybe with small change in process_client_pipe function or read_from_pipe. u could check cl is not zero , and this would prevent that segfault and make it work (but as said before , this is not that too simple and straight forward that i just add control with if (cl) . So , what do u think salem , Do u believe , with some change in read_from_pipe , or process_client_pipe or maybe some change in cleanup_thread , could prevent this segfault?

What happens if the killed client happens to be the first one on the list?

if u meant , put those line before the loop "for" , i had tried this , the result is the same , get segfault. if any other test , .. is needed , please let me know . would appreciate any idea
Regards

@Salem

just as my lasy comment on this thread. i just want to keep on with on old original read_from_pipe , write_to_pipe code (due the segfault) , i just want to move cs_malloc from write_to_pipe to read_from_pipe , without any change on memcpy , .. (the reason is , after this change , app would not get that segfault , and also since in entier source code of the programm free is called after each read_from_pipe , then resouce usages become optimized. i had tried in different way for this small change with no luck (get black screen) , would u please tell me how i could just move malloc from write_to_pipe to read_from_pipe? even if won't work ,i won't ask anymore question on this subject and thread. just want to give my last shot (last try to see this simple move could make any resource usages better or not)


P.S : here are original code of those 2 functions (i just need to move cs_malloc in write_to_pipe from line 30 to line read_from_pipe) :

Code:

int32_t write_to_pipe(int32_t fd, int32_t id, uchar *data, int32_t n)


{


    if (!fd) {


        cs_log("write_to_pipe: fd==0 id: %d", id);


        return -1;


    }

 

    //cs_debug_mask(D_TRACE,  "write to pipe %d (%s) thread: %8X to %8X", fd, PIP_ID_TXT[id],  pthread_self(), get_thread_by_pipefd(fd)->thread);
 
    uchar buf[3+sizeof(void*)];
 
    // fixme
    // copy data to allocated memory
    // needed for compatibility
    // need to be freed after read_from_pipe
 
    void *d;
    if(!cs_malloc(&d, n, -1)) return -1;
    memcpy(d, data, n);
 
    if ((id<0) || (id>PIP_ID_MAX))
        return(PIP_ID_ERR);
 
    memcpy(buf, PIP_ID_TXT[id], 3);
    memcpy(buf+3, &d, sizeof(void*));
 
    n=3+sizeof(void*);
 
    return(write(fd, buf, n));


}

Code:

 int32_t read_from_pipe(struct s_client *client, uchar **data) {     
if(!client)          return PIP_ID_ERR;      
 int32_t n, rc, fd = client->fd_m2c_c;      
 intptr_t hdr=0;     
 uchar buf[3+sizeof(void*)];  
    memset(buf, 0, sizeof(buf));      *data=(uchar *)0; 
     rc=PIP_ID_NUL;    
  cs_writelock(&client->pipelock);  
     n=read(fd, buf, sizeof(buf));   
    if(client->pipecnt >= n) client->pipecnt -= n;      
 cs_writeunlock(&client->pipelock);    
   if (n==sizeof(buf)) {      

   memcpy(&hdr, buf+3, sizeof(void*));   

   } else {   

      cs_log("WARNING: pipe header to small !");   
       return PIP_ID_ERR;     
 }   
     int32_t l;   
   for (l=0; (rc<0) && (PIP_ID_TXT[l]); l++)       
   if (!memcmp(buf, PIP_ID_TXT[l], 3))     
         rc=l;     
 if (rc<0) {          cs_log("WARNING: pipe garbage from pipe %i", fd);     
     return PIP_ID_ERR; 
    }    
  *data = (void*)hdr;     
 return(rc);

 }