Looks great! Nice explanation too :)
Robert. On Aug 20, 2009, at 5:34 PM, Marcel Telka wrote: > Hi all, > > Please code review following simple fix (6 lines changed): > http://cr.opensolaris.org/~aragorn/6873852/ > > Background (root cause): > ======================== > > The problem is with uninitialized ksum_kstat in nfsstat.c file. > > First, ksum_kstat is allocated using malloc() in setup(). Then, > cn_print() is > called and in turn the nfsstat_kstat_sum() is invoked. As a third > parameter in > the nfsstat_kstat_sum() call the ksum_kstat is passed. Please note the > cn_print()/nfsstat_kstat_sum() pair can be called multiple times in > a loop. > > nfsstat_kstat_sum() assumes several things for its third parameter > (named sum > inside the nfsstat_kstat_sum() implementation) which are not always > true: > > 1. sum is allocated (it is not NULL); Why? malloc() in setup() can > fail and > return NULL > > 2. sum->ks_data is NULL when nfsstat_kstat_sum() is called first > time; Why? > nobody is zeroing the ksum_kstat after malloc() in setup() > > The assumption #2 caused the core reported here. libumem filled the > ksum_kstat > in the malloc() by some data, then the nfsstat_kstat_sum() did not > called > nfsstat_kstat_copy() leaving sum->ks_data to point to nowhere. And > finally we > failed and cored in following "for" statement while trying to > dereference > sum->ks_data (aliased here to knpsum). > > 999 nfsstat_kstat_sum(kstat_t *kstat1, kstat_t *kstat2, kstat_t > *sum) > 1000 { > 1001 int i; > 1002 kstat_named_t *knp1, *knp2, *knpsum; > 1003 if (kstat1 == NULL || kstat2 == NULL) > 1004 return; > 1005 > 1006 knp1 = KSTAT_NAMED_PTR(kstat1); > 1007 knp2 = KSTAT_NAMED_PTR(kstat2); > 1008 if (sum->ks_data == NULL) > 1009 nfsstat_kstat_copy(kstat1, sum, 0); > 1010 knpsum = KSTAT_NAMED_PTR(sum); > 1011 > 1012 for (i = 0; i < (kstat1->ks_ndata); i++) > 1013 knpsum[i].value.ui64 = knp1[i].value.ui64 + > knp2[i].value.ui64; > 1014 } > > The change aims to fix both wrong assumptions in nfsstat_kstat_sum() > - see > above. In addition one small bug in safe_zalloc() is fixed too. > > > > Thanks for the review. > > -- > Marcel Telka > Solaris RPE > _______________________________________________ > nfs-discuss mailing list > nfs-discuss at opensolaris.org
