[support] Memory corruption in ha.c?

[Romain KUNTZ]

Hi Tero,

Sorry for this delayed reply! Comments inline.

On 2008/08/11, at 2:32, Tero Kauppinen (JO/LMF) wrote:
> 1) I suspect that there can be a possibility for a memory corruption  
> in ha.c. Referring to the file at:
>
> http://www.linux-ipv6.org/gitweb/gitweb.cgi?p=gitroot/mipv6-daemon.git;a=blob;h=f6d5cda76c7f72b48e490b874f3f6e53974df5ea;hb=master;f=src/ha.c
>
> Function ha_recv_bu_worker on line:
>
> 874         *(arg->statusp) = status;
>
> When a ha_recv_bu_worker thread is created in ha_recv_bu_main, it  
> stores a pointer to a local status variable (line 895: int status =  
> 0; and line 934: arg->statusp = &status;) which is then used at that  
> above 874 line to return the status value. However, if no join is  
> requested (i.e. !(arg->flags & HA_BU_F_THREAD_JOIN), ha_recv_bu_main  
> exists and thus a pointer to the local variable points somewhere in  
> the memory which might no longer be usable.

Good catch. Actually the HA_BU_F_THREAD_JOIN flag is not even ever  
set, so what you describe will always happen.

> 2) in ha_recv_bu_worker free(arg) is called already at line 863,  
> which means arg is used when the memory it points to is already  
> freed (line 874).

Yes, another great catch!

> Both 1) and 2) could be fixed in the following way:
>
> out:
>        pthread_mutex_lock(&bu_worker_mutex);
>        if (!list_empty(&bu_worker_list)) {
>                struct list_head *l = bu_worker_list.next;
>                list_del(l);
>                free(arg);
>                arg = list_entry(l, struct ha_recv_bu_args, list);
>                pthread_mutex_unlock(&bu_worker_mutex);
>                goto restart;
>        }
>        if (--bu_worker_count == 0)
>                pthread_cond_signal(&cond);
>
>        /* statusp points to a variable in the ha_recv_bu_main 		
>         * function's stack and MUST not be used unless the
>         * function is waiting for the thread in pthread_join().
>         */
>        if (arg->flags & HA_BU_F_THREAD_JOIN)
>                *(arg->statusp) = status;
>        free(arg);
>        pthread_mutex_unlock(&bu_worker_mutex);
>        pthread_exit(NULL);

What you propose is IMHO the right way to do. I'll send a mail ASAP to  
the UMIP maintainers (and CC you) in order to try to push your patch  
to the main UMIP repository.

Thanks,
-- 
Romain KUNTZ
kuntz at lsiit.u-strasbg.fr
LSIIT - Networks and Protocols Team
http://clarinet.u-strasbg.fr/~kuntz/