Thread: [Ipsec-tools-devel] [PATCH] Fix dpd_r_u sched memory corruption
Brought to you by:
mit_warlord,
netbsd
From: <tim...@ik...> - 2008-01-07 11:32:49
Attachments:
dpd_fix.diff
|
Hi, Explicitly reset iph1->dpd_r_u in the scheduler callback. Otherwise it might be left pointing to freed memory as the callback might return early if some checks fails. This results in memory corruption/segmentation fault when the ph1 is deleted and delph1() calls SCHED_KILL(iph1->dpd_r_u) while dpd_r_u is pointing to already freed area. Cheers, Timo |
From: VANHULLEBUS Y. <va...@fr...> - 2008-01-11 14:28:56
|
On Mon, Jan 07, 2008 at 01:33:22PM +0200, Timo Ter?s wrote: > Hi, Hi. > Explicitly reset iph1->dpd_r_u in the scheduler callback. Otherwise > it might be left pointing to freed memory as the callback might > return early if some checks fails. This results in memory > corruption/segmentation fault when the ph1 is deleted and delph1() > calls SCHED_KILL(iph1->dpd_r_u) while dpd_r_u is pointing to already > freed area. Right. But we don't really need to call SCHED_KILL, as schedular will free the struct_shed as soon as the callback is finished, we just need to set iph1->dpd_r_u to NULL. That's what I just commited on both HEAD and V0.7 branch, thanks for the patch. Yvan. |
From: <tim...@ik...> - 2008-01-11 14:36:34
|
Hi, VANHULLEBUS Yvan wrote: > On Mon, Jan 07, 2008 at 01:33:22PM +0200, Timo Ter?s wrote: >> Explicitly reset iph1->dpd_r_u in the scheduler callback. Otherwise >> it might be left pointing to freed memory as the callback might >> return early if some checks fails. This results in memory >> corruption/segmentation fault when the ph1 is deleted and delph1() >> calls SCHED_KILL(iph1->dpd_r_u) while dpd_r_u is pointing to already >> freed area. > > Right. > > But we don't really need to call SCHED_KILL, as schedular will free > the struct_shed as soon as the callback is finished, we just need to > set iph1->dpd_r_u to NULL. Yes. This exactly what SCHED_KILL does, marks the entry dead and assigns the pointer to NULL. Usage of SCHED_KILL is consistent to other timer callbacks in isakmp.c e.g. isakmp_ph1expire() does it so too. Though now that I look other places also the NULL assignment is used sometimes. Confusing. > That's what I just commited on both HEAD and V0.7 branch, thanks for > the patch. Thanks. - Timo |