The following reply was made to PR kern/36673; it has been noted by GNATS.

From: Juan RP <juan@xtrarom.org>
To: gnats-bugs@NetBSD.org
Cc: yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi)
Subject: Re: kern/36673: dubious code in sysmon_envsys
Date: Sat, 21 Jul 2007 18:59:38 +0200

 On Sat, 21 Jul 2007 16:30:02 +0000 (UTC)
 yamt@mwd.biglobe.ne.jp (YAMAMOTO Takashi) wrote:
 
 >  to me, the locking here seems far from obvious.  for example, to me,
 >  
 >  - it isn't clear which of sme_mtx or sme_list_mtx protects
 > sysmon_envsys_list. 
 
 This should be sme_list_mtx for the LIST. I fixed most cases, but
 perhaps there are still some, I'll fix.
 
 >  - it isn't clear if it's safe to drop sme_mtx in the middle of
 >   sme_make_dictionary.
  
 Can I allocate/deallocate memory with kmem(9) with a mutex held?
 someone said that it's not possible because it could cause a deadlock
 or something.
 
 >  although i'm not sure what you want to do here,
 >  a usual way to "skip" an entry in a loop is just "continue".
 >  
 >  	for (i = 0; i < sme->sme_nsensors; i++) {
 >  		edata = &sme->sme_sensor_data[i];
 >  		if (edata->flags & ENVSYS_FDUPDESC) {
 >  			continue;
 >  		}
 >  
 >  can you explain why you want to modify sme_nsensors here?
 >  it isn't obvious to me.
 
 Sure. In sme_make_dictionary the sensors with a duplicate description
 do not create another dictionary in the array, so the number of
 dictionaries in the array doesn't match with sme_nsensors.
 
 So let's see this code in sme_update_dictionary():
 
         for (i = 0; i < sme->sme_nsensors; i++) {
                 edata = &sme->sme_sensor_data[i];
                 if (edata->flags & ENVSYS_FDUPDESC) {
                         --sme->sme_nsensors;
                         continue;
                 }
 
 With the code in sys/lkm/misc/envsys2, there are 15 sensors in total
 and sensors with index 13 and 14 have a duplicate description.
 
 If sme_nsensors is not decreased the following code:
 
                 /* retrieve sensor's dictionary. */
                 dict = prop_array_get(array, i);
                 if (prop_object_type(dict) != PROP_TYPE_DICTIONARY) {
                         DPRINTF(("%s: not a dictionary (%d:%s)\n",
                             __func__, edata->sensor, sme->sme_name));
                         return EINVAL;
                 }
 
 will fail, because 'i' will still try to access to the index specified
 in sme_nsensors, but the dictionary doesn't exist and it will fail.
 
 Is it clear for you now?
 
 -- 
 Juan Romero Pardines 	- The NetBSD Project
 http://plog.xtrarom.org 	- NetBSD/pkgsrc news in Spanish