[CHECKER] 32 Memory Leaks on Error Paths

[CHECKER] 32 Memory Leaks on Error Paths

[David Yu Chen]

Hi All,

I'm with the Stanford Meta-level Compilation research group, and I
have a set of memory leaks on error paths for the 2.6.0-test5 kernel.
(I also have error reports for 2.4.18 and a couple other kernels if
anyone is interested).

There may be one or more "GOTO -->" markers showing the different
paths of execution that can occur between where the memory is
allocated and where the function returns.

My checker identifies error paths with a learning algorithm on
features surrounding goto and return statements.  I'd greatly
appreciate any comments or confirmation on these bugs.

Thanks!

---
David Yu Chen
http://www.stanford.edu/~dychen/

---------------------------------------------------------

Count = 32
# | File
1 | drivers/char/vt_ioctl.c
1 | drivers/media/video/bttv-risc.c
2 | drivers/mtd/chips/cfi_cmdset_0020.c
1 | drivers/mtd/mtdblock.c
1 | drivers/net/irda/ali-ircc.c
1 | drivers/net/irda/nsc-ircc.c
2 | drivers/pci/hotplug/ibmphp_ebda.c
1 | drivers/scsi/NCR_Q720.c
1 | drivers/scsi/scsi_debug.c
1 | drivers/telephony/ixj_pcmcia.c
2 | drivers/usb/class/usb-midi.c
1 | drivers/usb/input/hiddev.c
1 | fs/afs/cell.c
2 | fs/jffs2/scan.c
1 | fs/nfsd/nfsctl.c
2 | net/ipv4/igmp.c
1 | net/irda/irlmp.c
1 | net/sunrpc/auth_gss/gss_mech_switch.c
1 | net/sunrpc/auth_gss/gss_pseudoflavors.c
1 | security/selinux/selinuxfs.c
1 | security/selinux/ss/mls.c
4 | security/selinux/ss/policydb.c
1 | sound/oss/emu10k1/midi.c
1 | sound/oss/ymfpci.c

---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/char/vt_ioctl.c]
[FUNC:  do_kdsk_ioctl]
[LINES: 133-150]
[VAR:   key_map]
 128:
 129:			if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
 130:			    !capable(CAP_SYS_RESOURCE))
 131:				return -EPERM;
 132:
START -->
 133:			key_map = (ushort *) kmalloc(sizeof(plain_map),
 134:						     GFP_KERNEL);
 135:			if (!key_map)
 136:				return -ENOMEM;
 137:			key_maps[s] = key_map;
 138:			key_map[0] = U(K_ALLOCATED);
        ... DELETED 6 lines ...
 145:			break;	/* nothing to do */
 146:		/*
 147:		 * Attention Key.
 148:		 */
 149:		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
END -->
 150:			return -EPERM;
 151:		key_map[i] = U(v);
 152:		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
 153:			compute_shiftstate();
 154:		break;
 155:	}
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/media/video/bttv-risc.c]
[FUNC:  bttv_risc_overlay]
[LINES: 214-223]
[VAR:   skips]
 209:	struct btcx_skiplist *skips;
 210:	u32 *rp,ri,ra;
 211:	u32 addr;
 212:
 213:	/* skip list for window clipping */
START -->
 214:	if (NULL == (skips = kmalloc(sizeof(*skips) * ov->nclips,GFP_KERNEL)))
 215:		return -ENOMEM;
 216:	
 217:	/* estimate risc mem: worst case is (clip+1) * lines instructions
 218:	   + sync + jump (all 2 dwords) */
 219:	instructions  = (ov->nclips + 1) *
 220:		((skip_even || skip_odd) ? ov->w.height>>1 :  ov->w.height);
 221:	instructions += 2;
 222:	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0)
END -->
 223:		return rc;
 224:
 225:	/* sync instruction */
 226:	rp = risc->cpu;
 227:	*(rp++) = cpu_to_le32(BT848_RISC_SYNC|BT848_FIFO_STATUS_FM1);
 228:	*(rp++) = cpu_to_le32(0);
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
[FUNC:  cfi_staa_setup]
[LINES: 191-211]
[VAR:   mtd]
 186:	struct mtd_info *mtd;
 187:	unsigned long offset = 0;
 188:	int i,j;
 189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
 190:
START -->
 191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
 192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
 193:
 194:	if (!mtd) {
 195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
 196:		kfree(cfi->cmdset_priv);
        ... DELETED 9 lines ...
 206:	mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info) 
 207:			* mtd->numeraseregions, GFP_KERNEL);
 208:	if (!mtd->eraseregions) { 
 209:		printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
 210:		kfree(cfi->cmdset_priv);
END -->
 211:		return NULL;
 212:	}
 213:	
 214:	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
 215:		unsigned long ernum, ersize;
 216:		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
[FUNC:  cfi_staa_setup]
[LINES: 191-235]
[VAR:   mtd]
 186:	struct mtd_info *mtd;
 187:	unsigned long offset = 0;
 188:	int i,j;
 189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
 190:
START -->
 191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
 192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
 193:
 194:	if (!mtd) {
 195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
 196:		kfree(cfi->cmdset_priv);
        ... DELETED 33 lines ...
 230:		if (offset != devsize) {
 231:			/* Argh */
 232:			printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
 233:			kfree(mtd->eraseregions);
 234:			kfree(cfi->cmdset_priv);
END -->
 235:			return NULL;
 236:		}
 237:
 238:		for (i=0; i<mtd->numeraseregions;i++){
 239:			printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
 240:			       i,mtd->eraseregions[i].offset,
---------------------------------------------------------

looks like checking for mtdblks instead of mtdblk
[FILE:  2.6.0-test5/drivers/mtd/mtdblock.c]
[FUNC:  mtdblock_open]
[LINES: 277-279]
[VAR:   mtdblk]
 272:		mtdblks[dev]->count++;
 273:		return 0;
 274:	}
 275:	
 276:	/* OK, it's not open. Create cache info for it */
START -->
 277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
 278:	if (!mtdblks)
END -->
 279:		return -ENOMEM;
 280:
 281:	memset(mtdblk, 0, sizeof(*mtdblk));
 282:	mtdblk->count = 1;
 283:	mtdblk->mtd = mtd;
 284:
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/net/irda/ali-ircc.c]
[FUNC:  ali_ircc_open]
[LINES: 259-337]
[VAR:   self]
 254:	/* Set FIR FIFO and DMA Threshold */
 255:	if ((ali_ircc_setup(info)) == -1)
 256:		return -1;
 257:		
 258:	/* Allocate new instance of the driver */
START -->
 259:	self = kmalloc(sizeof(struct ali_ircc_cb), GFP_KERNEL);
 260:	if (self == NULL) 
 261:	{
 262:		ERROR("%s(), can't allocate memory for control block!\n", __FUNCTION__);
 263:		return -ENOMEM;
 264:	}
        ... DELETED 67 lines ...
 332:	self->tx_fifo.len = self->tx_fifo.ptr = self->tx_fifo.free = 0;
 333:	self->tx_fifo.tail = self->tx_buff.head;
 334:
 335:	if (!(dev = dev_alloc("irda%d", &err))) {
 336:		ERROR("%s(), dev_alloc() failed!\n", __FUNCTION__);
END -->
 337:		return -ENOMEM;
 338:	}
 339:
 340:	dev->priv = (void *) self;
 341:	self->netdev = dev;
 342:	
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/net/irda/nsc-ircc.c]
[FUNC:  nsc_ircc_open]
[LINES: 265-341]
[VAR:   self]
 260:		return -1;
 261:
 262:	MESSAGE("%s, driver loaded (Dag Brattli)\n", driver_name);
 263:
 264:	/* Allocate new instance of the driver */
START -->
 265:	self = kmalloc(sizeof(struct nsc_ircc_cb), GFP_KERNEL);
 266:	if (self == NULL) {
 267:		ERROR("%s(), can't allocate memory for "
 268:		       "control block!\n", __FUNCTION__);
 269:		return -ENOMEM;
 270:	}
        ... DELETED 65 lines ...
 336:	self->tx_fifo.len = self->tx_fifo.ptr = self->tx_fifo.free = 0;
 337:	self->tx_fifo.tail = self->tx_buff.head;
 338:
 339:	if (!(dev = dev_alloc("irda%d", &err))) {
 340:		ERROR("%s(), dev_alloc() failed!\n", __FUNCTION__);
END -->
 341:		return -ENOMEM;
 342:	}
 343:
 344:	dev->priv = (void *) self;
 345:	self->netdev = dev;
 346:
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/pci/hotplug/ibmphp_ebda.c]
[FUNC:  ebda_rsrc_controller]
[LINES: 857-1064]
[VAR:   bus_info_ptr1]
 852:
 853:			// create bus_info lined list --- if only one slot per bus: slot_min = slot_max 
 854:
 855:			bus_info_ptr2 = ibmphp_find_same_bus_num (slot_ptr->slot_bus_num);
 856:			if (!bus_info_ptr2) {
START -->
 857:				bus_info_ptr1 = (struct bus_info *) kmalloc (sizeof (struct bus_info), GFP_KERNEL);
 858:				if (!bus_info_ptr1) {
 859:					rc = -ENOMEM;
 860:					goto error_no_hp_slot;
 861:				}
 862:				memset (bus_info_ptr1, 0, sizeof (struct bus_info));
        ... DELETED 79 lines ...
 942:				hpc_ptr->irq = readb (io_mem + addr + 5);
 943:				addr += 6;
 944:				break;
 945:			default:
 946:				rc = -ENODEV;
GOTO -->
 947:				goto error_no_hp_slot;
 948:		}
 949:
 950:		//reorganize chassis' linked list
 951:		combine_wpg_for_chassis ();
 952:		combine_wpg_for_expansion ();
        ... DELETED 106 lines ...
1059:	kfree (hp_slot_ptr);
1060:error_no_hp_slot:
1061:	free_ebda_hpc (hpc_ptr);
1062:error_no_hpc:
1063:	iounmap (io_mem);
END -->
1064:	return rc;
1065:}
1066:
1067:/* 
1068: * map info (bus, devfun, start addr, end addr..) of i/o, memory,
1069: * pfm from the physical addr to a list of resource.
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/pci/hotplug/ibmphp_ebda.c]
[FUNC:  ebda_rsrc_controller]
[LINES: 981-1064]
[VAR:   tmp_slot]
 976:			if (!hp_slot_ptr->name) {
 977:				rc = -ENOMEM;
 978:				goto error_no_hp_name;
 979:			}
 980:
START -->
 981:			tmp_slot = kmalloc(sizeof(*tmp_slot), GFP_KERNEL);
 982:			if (!tmp_slot) {
 983:				rc = -ENOMEM;
 984:				goto error_no_slot;
 985:			}
 986:			memset(tmp_slot, 0, sizeof(*tmp_slot));
        ... DELETED 17 lines ...
1004:			tmp_slot->bus = hpc_ptr->slots[index].slot_bus_num;
1005:
1006:			bus_info_ptr1 = ibmphp_find_same_bus_num (hpc_ptr->slots[index].slot_bus_num);
1007:			if (!bus_info_ptr1) {
1008:				rc = -ENODEV;
GOTO -->
1009:				goto error;
1010:			}
1011:			tmp_slot->bus_on = bus_info_ptr1;
1012:			bus_info_ptr1 = NULL;
1013:			tmp_slot->ctrl = hpc_ptr;
1014:
        ... DELETED 44 lines ...
1059:	kfree (hp_slot_ptr);
1060:error_no_hp_slot:
1061:	free_ebda_hpc (hpc_ptr);
1062:error_no_hpc:
1063:	iounmap (io_mem);
END -->
1064:	return rc;
1065:}
1066:
1067:/* 
1068: * map info (bus, devfun, start addr, end addr..) of i/o, memory,
1069: * pfm from the physical addr to a list of resource.
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/scsi/NCR_Q720.c]
[FUNC:  NCR_Q720_probe]
[LINES: 153-182]
[VAR:   p]
 148:	__u8 pos2, pos4, asr2, asr9, asr10;
 149:	__u16 io_base;
 150:	__u32 base_addr, mem_size;
 151:	__u32 mem_base;
 152:
START -->
 153:	p = kmalloc(sizeof(*p), GFP_KERNEL);
 154:	if (!p)
 155:		return -ENOMEM;
 156:
 157:	memset(p, 0, sizeof(*p));
 158:	pos2 = mca_device_read_pos(mca_dev, 2);
        ... DELETED 18 lines ...
 177:
 178:	/* sanity check I/O mapping */
 179:	i = inb(io_base) | (inb(io_base+1)<<8);
 180:	if(i != NCR_Q720_MCA_ID) {
 181:		printk(KERN_ERR "NCR_Q720, adapter failed to I/O map registers correctly at 0x%x(0x%x)\n", io_base, i);
END -->
 182:		return -ENODEV;
 183:	}
 184:
 185:	/* Phase II, find the ram base and memory map the board register */
 186:	pos4 = inb(io_base + 4);
 187:	/* enable streaming data */
---------------------------------------------------------

Leaks memory if kmalloc fails between 0 .. devs_per_host
[FILE:  2.6.0-test5/drivers/scsi/scsi_debug.c]
[FUNC:  sdebug_add_adapter]
[LINES: 1612-1652]
[VAR:   sdbg_devinfo]
1607:        memset(sdbg_host, 0, sizeof(*sdbg_host));
1608:        INIT_LIST_HEAD(&sdbg_host->dev_info_list);
1609:
1610:	devs_per_host = scsi_debug_num_tgts * scsi_debug_max_luns;
1611:        for (k = 0; k < devs_per_host; k++) {
START -->
1612:                sdbg_devinfo = kmalloc(sizeof(*sdbg_devinfo),GFP_KERNEL);
1613:                if (NULL == sdbg_devinfo) {
1614:                        printk(KERN_ERR "%s: out of memory at line %d\n",
1615:                               __FUNCTION__, __LINE__);
1616:                        error = -ENOMEM;
GOTO -->
1617:			goto clean1;
        ... DELETED 29 lines ...
1647:		kfree(sdbg_devinfo);
1648:	}
1649:
1650:clean1:
1651:	kfree(sdbg_host);
END -->
1652:        return error;
1653:}
1654:
1655:static void sdebug_remove_adapter()
1656:{
1657:        struct sdebug_host_info * sdbg_host = NULL;
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/telephony/ixj_pcmcia.c]
[FUNC:  ixj_attach]
[LINES: 53-64]
[VAR:   link]
  48:	client_reg_t client_reg;
  49:	dev_link_t *link;
  50:	int ret;
  51:	DEBUG(0, "ixj_attach()\n");
  52:	/* Create new ixj device */
START -->
  53:	link = kmalloc(sizeof(struct dev_link_t), GFP_KERNEL);
  54:	if (!link)
  55:		return NULL;
  56:	memset(link, 0, sizeof(struct dev_link_t));
  57:	link->io.Attributes1 = IO_DATA_PATH_WIDTH_8;
  58:	link->io.Attributes2 = IO_DATA_PATH_WIDTH_8;
  59:	link->io.IOAddrLines = 3;
  60:	link->conf.Vcc = 50;
  61:	link->conf.IntType = INT_MEMORY_AND_IO;
  62:	link->priv = kmalloc(sizeof(struct ixj_info_t), GFP_KERNEL);
  63:	if (!link->priv)
END -->
  64:		return NULL;
  65:	memset(link->priv, 0, sizeof(struct ixj_info_t));
  66:	/* Register with Card Services */
  67:	link->next = dev_list;
  68:	dev_list = link;
  69:	client_reg.dev_info = &dev_info;
---------------------------------------------------------

Leaks if devices == 0 ?  Error_end only frees mdevs if (devices > 0), 
but for mdevs=kmalloc(0), the slab allocator may still actually return memory
[FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
[FUNC:  alloc_usb_midi_device]
[LINES: 1621-1772]
[VAR:   mdevs]
1616:	devices = inDevs > outDevs ? inDevs : outDevs;
1617:	devices = maxdevices > devices ? devices : maxdevices;
1618:
1619:	/* obtain space for device name (iProduct) if not known. */
1620:	if ( ! u->deviceName ) {
START -->
1621:		mdevs = (struct usb_mididev **)
1622:			kmalloc(sizeof(struct usb_mididevs *)*devices
1623:				+ sizeof(char) * 256, GFP_KERNEL);
1624:	} else {
1625:		mdevs = (struct usb_mididev **)
1626:			kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
        ... DELETED 83 lines ...
1710:		}
1711:		mout = mouts[outEndpoint];
1712:
1713:		mdevs[i] = allocMidiDev( s, min, mout, inCableId, outCableId );
1714:		if ( mdevs[i] == NULL )
GOTO -->
1715:			goto error_end;
1716:
1717:	}
1718:
1719:	/* Success! */
1720:	for ( i=0 ; i<devices ; i++ ) {
        ... DELETED 46 lines ...
1767:		if ( mouts[i] != NULL ) {
1768:			remove_midi_out_endpoint( mouts[i] );
1769:		}
1770:	}
1771:
END -->
1772:	return -ENOMEM;
1773:}
1774:
1775:/* ------------------------------------------------------------------------- */
1776:
1777:/** Attempt to scan YAMAHA's device descriptor and detect correct values of
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
[FUNC:  alloc_usb_midi_device]
[LINES: 1625-1772]
[VAR:   mdevs]
1620:	if ( ! u->deviceName ) {
1621:		mdevs = (struct usb_mididev **)
1622:			kmalloc(sizeof(struct usb_mididevs *)*devices
1623:				+ sizeof(char) * 256, GFP_KERNEL);
1624:	} else {
START -->
1625:		mdevs = (struct usb_mididev **)
1626:			kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
1627:	}
1628:
1629:	if ( !mdevs ) {
1630:		/* devices = 0; */
        ... DELETED 79 lines ...
1710:		}
1711:		mout = mouts[outEndpoint];
1712:
1713:		mdevs[i] = allocMidiDev( s, min, mout, inCableId, outCableId );
1714:		if ( mdevs[i] == NULL )
GOTO -->
1715:			goto error_end;
1716:
1717:	}
1718:
1719:	/* Success! */
1720:	for ( i=0 ; i<devices ; i++ ) {
        ... DELETED 46 lines ...
1767:		if ( mouts[i] != NULL ) {
1768:			remove_midi_out_endpoint( mouts[i] );
1769:		}
1770:	}
1771:
END -->
1772:	return -ENOMEM;
1773:}
1774:
1775:/* ------------------------------------------------------------------------- */
1776:
1777:/** Attempt to scan YAMAHA's device descriptor and detect correct values of
---------------------------------------------------------

[FILE:  2.6.0-test5/drivers/usb/input/hiddev.c]
[FUNC:  hiddev_connect]
[LINES: 723-730]
[VAR:   hiddev]
 718:			break;
 719:
 720:	if (i == hid->maxcollection && (hid->quirks & HID_QUIRK_HIDDEV) == 0)
 721:		return -1;
 722:
START -->
 723: 	if (!(hiddev = kmalloc(sizeof(struct hiddev), GFP_KERNEL)))
 724:		return -1;
 725:	memset(hiddev, 0, sizeof(struct hiddev));
 726:
 727: 	retval = usb_register_dev(&hiddev->intf, &hiddev_class);
 728:	if (retval) {
 729:		err("Not able to get a minor for this device.");
END -->
 730:		return -1;
 731:	}
 732:
 733:	init_waitqueue_head(&hiddev->wait);
 734:
 735: 	hiddev->minor = hiddev->intf.minor;
---------------------------------------------------------

[FILE:  2.6.0-test5/fs/afs/cell.c]
[FUNC:  afs_cell_create]
[LINES: 58-129]
[VAR:   cell]
  53:	if (!name) BUG(); /* TODO: want to look up "this cell" in the cache */
  54:
  55:	down_write(&afs_cells_sem);
  56:
  57:	/* allocate and initialise a cell record */
START -->
  58:	cell = kmalloc(sizeof(afs_cell_t) + strlen(name) + 1,GFP_KERNEL);
  59:	if (!cell) {
  60:		_leave(" = -ENOMEM");
  61:		return -ENOMEM;
  62:	}
  63:
        ... DELETED 25 lines ...
  89:
  90:		if (sscanf(vllist,"%u.%u.%u.%u",&a,&b,&c,&d)!=4)
  91:			goto badaddr;
  92:
  93:		if (a>255 || b>255 || c>255 || d>255)
GOTO -->
  94:			goto badaddr;
  95:
  96:		cell->vl_addrs[cell->vl_naddrs++].s_addr =
  97:			htonl((a<<24)|(b<<16)|(c<<8)|d);
  98:
  99:		if (cell->vl_naddrs>=16)
        ... DELETED 24 lines ...
 124: badaddr:
 125:	printk("kAFS: bad VL server IP address: '%s'\n",vllist);
 126: error:
 127:	up_write(&afs_cells_sem);
 128:	kfree(afs_cell_root);
END -->
 129:	return ret;
 130:} /* end afs_cell_create() */
 131:
 132:/*****************************************************************************/
 133:/*
 134: * initialise the cell database from module parameters
---------------------------------------------------------

[FILE:  2.6.0-test5/fs/jffs2/scan.c]
[FUNC:  jffs2_scan_medium]
[LINES: 98-109]
[VAR:   flashbuf]
  93:			buf_size = c->sector_size;
  94:		else
  95:			buf_size = PAGE_SIZE;
  96:
  97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
START -->
  98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
  99:		if (!flashbuf)
 100:			return -ENOMEM;
 101:	}
 102:
 103:	for (i=0; i<c->nr_blocks; i++) {
 104:		struct jffs2_eraseblock *jeb = &c->blocks[i];
 105:
 106:		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
 107:
 108:		if (ret < 0)
END -->
 109:			return ret;
 110:
 111:		ACCT_PARANOIA_CHECK(jeb);
 112:
 113:		/* Now decide which list to put it on */
 114:		switch(ret) {
---------------------------------------------------------

[FILE:  2.6.0-test5/fs/jffs2/scan.c]
[FUNC:  jffs2_scan_medium]
[LINES: 98-233]
[VAR:   flashbuf]
  93:			buf_size = c->sector_size;
  94:		else
  95:			buf_size = PAGE_SIZE;
  96:
  97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
START -->
  98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
  99:		if (!flashbuf)
 100:			return -ENOMEM;
 101:	}
 102:
 103:	for (i=0; i<c->nr_blocks; i++) {
        ... DELETED 124 lines ...
 228:	}
 229:	if (c->nr_erasing_blocks) {
 230:		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
 231:			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
 232:			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
END -->
 233:			return -EIO;
 234:		}
 235:		jffs2_erase_pending_trigger(c);
 236:	}
 237:	if (buf_size)
 238:		kfree(flashbuf);
---------------------------------------------------------

[FILE:  2.6.0-test5/fs/nfsd/nfsctl.c]
[FUNC:  TA_write]
[LINES: 105-120]
[VAR:   ar]
 100:	if (file->private_data) 
 101:		return -EINVAL; /* only one write allowed per open */
 102:	if (size > PAGE_SIZE - sizeof(struct argresp))
 103:		return -EFBIG;
 104:
START -->
 105:	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
 106:	if (!ar)
 107:		return -ENOMEM;
 108:	ar->size = 0;
 109:	down(&file->f_dentry->d_inode->i_sem);
 110:	if (file->private_data)
        ... DELETED 4 lines ...
 115:	if (rv) {
 116:		kfree(ar);
 117:		return rv;
 118:	}
 119:	if (copy_from_user(ar->data, buf, size))
END -->
 120:		return -EFAULT;
 121:	
 122:	rv =  write_op[ino](file, ar->data, size);
 123:	if (rv>0) {
 124:		ar->size = rv;
 125:		rv = size;
---------------------------------------------------------

[FILE:  2.6.0-test5/net/ipv4/igmp.c]
[FUNC:  igmpv3_newpack]
[LINES: 274-284]
[VAR:   skb]
 269:	struct sk_buff *skb;
 270:	struct rtable *rt;
 271:	struct iphdr *pip;
 272:	struct igmpv3_report *pig;
 273:
START -->
 274:	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
 275:	if (skb == NULL)
 276:		return 0;
 277:
 278:	{
 279:		struct flowi fl = { .oif = dev->ifindex,
 280:				    .nl_u = { .ip4_u = {
 281:				    .daddr = IGMPV3_ALL_MCR } },
 282:				    .proto = IPPROTO_IGMP };
 283:		if (ip_route_output_key(&rt, &fl))
END -->
 284:			return 0;
 285:	}
 286:	if (rt->rt_src == 0) {
 287:		ip_rt_put(rt);
 288:		return 0;
 289:	}
---------------------------------------------------------

[FILE:  2.6.0-test5/net/ipv4/igmp.c]
[FUNC:  igmpv3_newpack]
[LINES: 274-288]
[VAR:   skb]
 269:	struct sk_buff *skb;
 270:	struct rtable *rt;
 271:	struct iphdr *pip;
 272:	struct igmpv3_report *pig;
 273:
START -->
 274:	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
 275:	if (skb == NULL)
 276:		return 0;
 277:
 278:	{
 279:		struct flowi fl = { .oif = dev->ifindex,
        ... DELETED 3 lines ...
 283:		if (ip_route_output_key(&rt, &fl))
 284:			return 0;
 285:	}
 286:	if (rt->rt_src == 0) {
 287:		ip_rt_put(rt);
END -->
 288:		return 0;
 289:	}
 290:
 291:	skb->dst = &rt->u.dst;
 292:	skb->dev = dev;
 293:
---------------------------------------------------------

[FILE:  2.6.0-test5/net/irda/irlmp.c]
[FUNC:  irlmp_open_lsap]
[LINES: 161-183]
[VAR:   self]
 156:			return NULL;
 157:	} else if (irlmp_slsap_inuse(slsap_sel))
 158:		return NULL;
 159:
 160:	/* Allocate new instance of a LSAP connection */
START -->
 161:	self = kmalloc(sizeof(struct lsap_cb), GFP_ATOMIC);
 162:	if (self == NULL) {
 163:		ERROR("%s: can't allocate memory", __FUNCTION__);
 164:		return NULL;
 165:	}
 166:	memset(self, 0, sizeof(struct lsap_cb));
        ... DELETED 11 lines ...
 178:		self->dlsap_sel = LSAP_ANY;
 179:	/* self->connected = FALSE; -> already NULL via memset() */
 180:
 181:	init_timer(&self->watchdog_timer);
 182:
END -->
 183:	ASSERT(notify->instance != NULL, return NULL;);
 184:	self->notify = *notify;
 185:
 186:	self->lsap_state = LSAP_DISCONNECTED;
 187:
 188:	/* Insert into queue of unconnected LSAPs */
---------------------------------------------------------

[FILE:  2.6.0-test5/net/sunrpc/auth_gss/gss_mech_switch.c]
[FUNC:  gss_mech_register]
[LINES: 67-74]
[VAR:   gm]
  62:int
  63:gss_mech_register(struct xdr_netobj * mech_type, struct gss_api_ops * ops)
  64:{
  65:	struct gss_api_mech *gm;
  66:
START -->
  67:	if (!(gm = kmalloc(sizeof(*gm), GFP_KERNEL))) {
  68:		printk("Failed to allocate memory in gss_mech_register");
  69:		return -1;
  70:	}
  71:	gm->gm_oid.len = mech_type->len;
  72:	if (!(gm->gm_oid.data = kmalloc(mech_type->len, GFP_KERNEL))) {
  73:		printk("Failed to allocate memory in gss_mech_register");
END -->
  74:		return -1;
  75:	}
  76:	memcpy(gm->gm_oid.data, mech_type->data, mech_type->len);
  77:	/* We're counting the reference in the registered_mechs list: */
  78:	atomic_set(&gm->gm_count, 1);
  79:	gm->gm_ops = ops;
---------------------------------------------------------

[FILE:  2.6.0-test5/net/sunrpc/auth_gss/gss_pseudoflavors.c]
[FUNC:  gss_register_triple]
[LINES: 74-97]
[VAR:   triple]
  69:gss_register_triple(u32 pseudoflavor, struct gss_api_mech *mech,
  70:			  u32 qop, u32 service)
  71:{
  72:	struct sup_sec_triple *triple;
  73:
START -->
  74:	if (!(triple = kmalloc(sizeof(*triple), GFP_KERNEL))) {
  75:		printk("Alloc failed in gss_register_triple");
  76:		goto err;
  77:	}
  78:	triple->pseudoflavor = pseudoflavor;
  79:	triple->mech = gss_mech_get_by_OID(&mech->gm_oid);
        ... DELETED 1 lines ...
  81:	triple->service = service;
  82:
  83:	spin_lock(&registered_triples_lock);
  84:	if (do_lookup_triple_by_pseudoflavor(pseudoflavor)) {
  85:		printk("Registered pseudoflavor %d again\n", pseudoflavor);
GOTO -->
  86:		goto err_unlock;
  87:	}
  88:	list_add(&triple->triples, &registered_triples);
  89:	spin_unlock(&registered_triples_lock);
  90:	dprintk("RPC: registered pseudoflavor %d\n", pseudoflavor);
  91:
  92:	return 0;
  93:
  94:err_unlock:
  95:	spin_unlock(&registered_triples_lock);
  96:err:
END -->
  97:	return -1;
  98:}
  99:
 100:int
 101:gss_unregister_triple(u32 pseudoflavor)
 102:{
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/selinuxfs.c]
[FUNC:  TA_write]
[LINES: 253-269]
[VAR:   ar]
 248:	if (file->private_data)
 249:		return -EINVAL; /* only one write allowed per open */
 250:	if (size > PAYLOAD_SIZE - 1) /* allow one byte for null terminator */
 251:		return -EFBIG;
 252:
START -->
 253:	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
 254:	if (!ar)
 255:		return -ENOMEM;
 256:	memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
 257:	ar->size = 0;
 258:	down(&file->f_dentry->d_inode->i_sem);
        ... DELETED 5 lines ...
 264:	if (rv) {
 265:		kfree(ar);
 266:		return rv;
 267:	}
 268:	if (copy_from_user(ar->data, buf, size))
END -->
 269:		return -EFAULT;
 270:
 271:	rv =  write_op[ino](file, ar->data, size);
 272:	if (rv>0) {
 273:		ar->size = rv;
 274:		rv = size;
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/ss/mls.c]
[FUNC:  mls_read_user]
[LINES: 543-561]
[VAR:   r]
 538:		goto out;
 539:	}
 540:	nel = le32_to_cpu(buf[0]);
 541:	l = NULL;
 542:	for (i = 0; i < nel; i++) {
START -->
 543:		r = kmalloc(sizeof(*r), GFP_ATOMIC);
 544:		if (!r) {
 545:			rc = -ENOMEM;
 546:			goto out;
 547:		}
 548:		memset(r, 0, sizeof(*r));
 549:
 550:		rc = mls_read_range_helper(&r->range, fp);
 551:		if (rc)
GOTO -->
 552:			goto out;
 553:
 554:		if (l)
 555:			l->next = r;
 556:		else
 557:			usrdatum->ranges = r;
 558:		l = r;
 559:	}
 560:out:
END -->
 561:	return rc;
 562:}
 563:
 564:int mls_read_nlevels(struct policydb *p, void *fp)
 565:{
 566:	u32 *buf;
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
[FUNC:  policydb_read]
[LINES: 1232-1427]
[VAR:   c]
1227:			goto bad;
1228:		}
1229:		nel = le32_to_cpu(buf[0]);
1230:		l = NULL;
1231:		for (j = 0; j < nel; j++) {
START -->
1232:			c = kmalloc(sizeof(*c), GFP_KERNEL);
1233:			if (!c) {
1234:				rc = -ENOMEM;
1235:				goto bad;
1236:			}
1237:			memset(c, 0, sizeof(*c));
        ... DELETED 182 lines ...
1420:		}
1421:	}
1422:
1423:	rc = mls_read_trusted(p, fp);
1424:	if (rc)
GOTO -->
1425:		goto bad;
1426:out:
END -->
1427:	return rc;
1428:bad:
1429:	policydb_destroy(p);
1430:	goto out;
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
[FUNC:  policydb_read]
[LINES: 1334-1427]
[VAR:   newgenfs]
1329:	}
1330:	nel = le32_to_cpu(buf[0]);
1331:	genfs_p = NULL;
1332:	rc = -EINVAL;
1333:	for (i = 0; i < nel; i++) {
START -->
1334:		newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
1335:		if (!newgenfs) {
1336:			rc = -ENOMEM;
1337:			goto bad;
1338:		}
1339:		memset(newgenfs, 0, sizeof(*newgenfs));
        ... DELETED 5 lines ...
1345:		if (!buf)
1346:			goto bad;
1347:		newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
1348:		if (!newgenfs->fstype) {
1349:			rc = -ENOMEM;
GOTO -->
1350:			goto bad;
1351:		}
1352:		memcpy(newgenfs->fstype, buf, len);
1353:		newgenfs->fstype[len] = 0;
1354:		for (genfs_p = NULL, genfs = p->genfs; genfs;
1355:		     genfs_p = genfs, genfs = genfs->next) {
1356:			if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
1357:				printk(KERN_ERR "security:  dup genfs "
1358:				       "fstype %s\n", newgenfs->fstype);
GOTO -->
1359:				goto bad;
1360:			}
1361:			if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
1362:				break;
1363:		}
1364:		newgenfs->next = genfs;
        ... DELETED 57 lines ...
1422:
1423:	rc = mls_read_trusted(p, fp);
1424:	if (rc)
1425:		goto bad;
1426:out:
END -->
1427:	return rc;
1428:bad:
1429:	policydb_destroy(p);
1430:	goto out;
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
[FUNC:  policydb_read]
[LINES: 1374-1427]
[VAR:   newc]
1369:		buf = next_entry(fp, sizeof(u32));
1370:		if (!buf)
1371:			goto bad;
1372:		nel2 = le32_to_cpu(buf[0]);
1373:		for (j = 0; j < nel2; j++) {
START -->
1374:			newc = kmalloc(sizeof(*newc), GFP_KERNEL);
1375:			if (!newc) {
1376:				rc = -ENOMEM;
1377:				goto bad;
1378:			}
1379:			memset(newc, 0, sizeof(*newc));
        ... DELETED 14 lines ...
1394:			buf = next_entry(fp, sizeof(u32));
1395:			if (!buf)
1396:				goto bad;
1397:			newc->v.sclass = le32_to_cpu(buf[0]);
1398:			if (context_read_and_validate(&newc->context[0], p, fp))
GOTO -->
1399:				goto bad;
1400:			for (l = NULL, c = newgenfs->head; c;
1401:			     l = c, c = c->next) {
1402:				if (!strcmp(newc->u.name, c->u.name) &&
1403:				    (!c->v.sclass || !newc->v.sclass ||
1404:				     newc->v.sclass == c->v.sclass)) {
1405:					printk(KERN_ERR "security:  dup genfs "
1406:					       "entry (%s,%s)\n",
1407:					       newgenfs->fstype, c->u.name);
GOTO -->
1408:					goto bad;
1409:				}
1410:				len = strlen(newc->u.name);
1411:				len2 = strlen(c->u.name);
1412:				if (len > len2)
1413:					break;
        ... DELETED 8 lines ...
1422:
1423:	rc = mls_read_trusted(p, fp);
1424:	if (rc)
1425:		goto bad;
1426:out:
END -->
1427:	return rc;
1428:bad:
1429:	policydb_destroy(p);
1430:	goto out;
---------------------------------------------------------

[FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
[FUNC:  class_read]
[LINES: 759-851]
[VAR:   c]
 754:	}
 755:
 756:	lc = NULL;
 757:	rc = -EINVAL;
 758:	for (i = 0; i < ncons; i++) {
START -->
 759:		c = kmalloc(sizeof(*c), GFP_KERNEL);
 760:		if (!c) {
 761:			rc = -ENOMEM;
 762:			goto bad;
 763:		}
 764:		memset(c, 0, sizeof(*c));
        ... DELETED 64 lines ...
 829:				c->expr = e;
 830:			}
 831:			le = e;
 832:		}
 833:		if (depth != 0)
GOTO -->
 834:			goto bad;
 835:		if (lc) {
 836:			lc->next = c;
 837:		} else {
 838:			cladatum->constraints = c;
 839:		}
        ... DELETED 6 lines ...
 846:
 847:	rc = hashtab_insert(h, key, cladatum);
 848:	if (rc)
 849:		goto bad;
 850:out:
END -->
 851:	return rc;
 852:bad:
 853:	class_destroy(key, cladatum, NULL);
 854:	goto out;
 855:}
 856:
---------------------------------------------------------

[FILE:  2.6.0-test5/sound/oss/emu10k1/midi.c]
[FUNC:  emu10k1_seq_midi_open]
[LINES: 498-514]
[VAR:   midi_dev]
 493:	if (card->open_mode)		/* card is opened native */
 494:		return -EBUSY;
 495:			
 496:	DPF(2, "emu10k1_seq_midi_open()\n");
 497:	
START -->
 498:	if ((midi_dev = (struct emu10k1_mididevice *) kmalloc(sizeof(*midi_dev), GFP_KERNEL)) == NULL)
 499:		return -EINVAL;
 500:
 501:	midi_dev->card = card;
 502:	midi_dev->mistate = MIDIIN_STATE_STOPPED;
 503:	init_waitqueue_head(&midi_dev->oWait);
        ... DELETED 5 lines ...
 509:
 510:	dsCardMidiOpenInfo.refdata = (unsigned long) midi_dev;
 511:
 512:	if (emu10k1_mpuout_open(card, &dsCardMidiOpenInfo) < 0) {
 513:		ERROR();
END -->
 514:		return -ENODEV;
 515:	}
 516:
 517:	card->seq_mididev = midi_dev;
 518:		
 519:	return 0;
---------------------------------------------------------

[FILE:  2.6.0-test5/sound/oss/ymfpci.c]
[FUNC:  ymf_probe_one]
[LINES: 2530-2641]
[VAR:   codec]
2525:		printk(KERN_ERR "ymfpci: pci_enable_device failed\n");
2526:		return err;
2527:	}
2528:	base = pci_resource_start(pcidev, 0);
2529:
START -->
2530:	if ((codec = kmalloc(sizeof(ymfpci_t), GFP_KERNEL)) == NULL) {
2531:		printk(KERN_ERR "ymfpci: no core\n");
2532:		return -ENOMEM;
2533:	}
2534:	memset(codec, 0, sizeof(*codec));
2535:
        ... DELETED 11 lines ...
2547:		goto out_free;
2548:	}
2549:
2550:	if ((codec->reg_area_virt = ioremap(base, 0x8000)) == NULL) {
2551:		printk(KERN_ERR "ymfpci: unable to map registers\n");
GOTO -->
2552:		goto out_release_region;
2553:	}
2554:
2555:	pci_set_master(pcidev);
2556:
2557:	printk(KERN_INFO "ymfpci: %s at 0x%lx IRQ %d\n",
        ... DELETED 78 lines ...
2636: out_release_region:
2637:	release_mem_region(pci_resource_start(pcidev, 0), 0x8000);
2638: out_free:
2639:	if (codec->ac97_codec[0])
2640:		ac97_release_codec(codec->ac97_codec[0]);
END -->
2641:	return -ENODEV;
2642:}
2643:
2644:static void __devexit ymf_remove_one(struct pci_dev *pcidev)
2645:{
2646:	__u16 ctrl;
---------------------------------------------------------

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Neil Brown]

On Monday September 15, dychen@stanford.edu wrote:
> Hi All,
> 
> I'm with the Stanford Meta-level Compilation research group, and I
> have a set of memory leaks on error paths for the 2.6.0-test5 kernel.
> (I also have error reports for 2.4.18 and a couple other kernels if
> anyone is interested).
> 
> There may be one or more "GOTO -->" markers showing the different
> paths of execution that can occur between where the memory is
> allocated and where the function returns.
> 
> My checker identifies error paths with a learning algorithm on
> features surrounding goto and return statements.  I'd greatly
> appreciate any comments or confirmation on these bugs.
> 
> Thanks!

The nfsd/nfsctl.c one isn't actually a bug (though I had to think
about it a bit to be sure).

One of the "... DETELED 4 lines ..." is

		file->private_data = ar;

which takes a copy of "ar" into a structure that has a lifetime
greater than the function.
So when the "return -EFAULT" happends (at END -->), at is stored in
file->private_data, and a subsequent call to TA_release will free that
memory.

NeilBrown


> 
> [FILE:  2.6.0-test5/fs/nfsd/nfsctl.c]
> [FUNC:  TA_write]
> [LINES: 105-120]
> [VAR:   ar]
>  100:	if (file->private_data) 
>  101:		return -EINVAL; /* only one write allowed per open */
>  102:	if (size > PAGE_SIZE - sizeof(struct argresp))
>  103:		return -EFBIG;
>  104:
> START -->
>  105:	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  106:	if (!ar)
>  107:		return -ENOMEM;
>  108:	ar->size = 0;
>  109:	down(&file->f_dentry->d_inode->i_sem);
>  110:	if (file->private_data)
>         ... DELETED 4 lines ...
>  115:	if (rv) {
>  116:		kfree(ar);
>  117:		return rv;
>  118:	}
>  119:	if (copy_from_user(ar->data, buf, size))
> END -->
>  120:		return -EFAULT;
>  121:	
>  122:	rv =  write_op[ino](file, ar->data, size);
>  123:	if (rv>0) {
>  124:		ar->size = rv;
>  125:		rv = size;

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> 
> [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> [FUNC:  cfi_staa_setup]
> [LINES: 191-211]
> [VAR:   mtd]
>  186:	struct mtd_info *mtd;
>  187:	unsigned long offset = 0;
>  188:	int i,j;
>  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
>  190:
> START -->
>  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
>  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
>  193:
>  194:	if (!mtd) {
>  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
>  196:		kfree(cfi->cmdset_priv);
>         ... DELETED 9 lines ...
>  206:	mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info) 
>  207:			* mtd->numeraseregions, GFP_KERNEL);
>  208:	if (!mtd->eraseregions) { 
>  209:		printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
>  210:		kfree(cfi->cmdset_priv);
> END -->
>  211:		return NULL;
>  212:	}
>  213:	
>  214:	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
>  215:		unsigned long ernum, ersize;
>  216:		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;

Valid.

> [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> [FUNC:  cfi_staa_setup]
> [LINES: 191-235]
> [VAR:   mtd]
>  186:	struct mtd_info *mtd;
>  187:	unsigned long offset = 0;
>  188:	int i,j;
>  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
>  190:
> START -->
>  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
>  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
>  193:
>  194:	if (!mtd) {
>  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
>  196:		kfree(cfi->cmdset_priv);
>         ... DELETED 33 lines ...
>  230:		if (offset != devsize) {
>  231:			/* Argh */
>  232:			printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
>  233:			kfree(mtd->eraseregions);
>  234:			kfree(cfi->cmdset_priv);
> END -->
>  235:			return NULL;
>  236:		}
>  237:
>  238:		for (i=0; i<mtd->numeraseregions;i++){
>  239:			printk(KERN_DEBUG "%d: offset=0x%x,size=0x%x,blocks=%d\n",
>  240:			       i,mtd->eraseregions[i].offset,

Also valid.

> looks like checking for mtdblks instead of mtdblk
> [FILE:  2.6.0-test5/drivers/mtd/mtdblock.c]
> [FUNC:  mtdblock_open]
> [LINES: 277-279]
> [VAR:   mtdblk]
>  272:		mtdblks[dev]->count++;
>  273:		return 0;
>  274:	}
>  275:	
>  276:	/* OK, it's not open. Create cache info for it */
> START -->
>  277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>  278:	if (!mtdblks)
> END -->
>  279:		return -ENOMEM;
>  280:
>  281:	memset(mtdblk, 0, sizeof(*mtdblk));
>  282:	mtdblk->count = 1;
>  283:	mtdblk->mtd = mtd;
>  284:

Invalid.  This is quite an obvious false positive, at least if your
algorithm checks for possible value ranges.

> [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> [FUNC:  jffs2_scan_medium]
> [LINES: 98-109]
> [VAR:   flashbuf]
>   93:			buf_size = c->sector_size;
>   94:		else
>   95:			buf_size = PAGE_SIZE;
>   96:
>   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> START -->
>   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
>   99:		if (!flashbuf)
>  100:			return -ENOMEM;
>  101:	}
>  102:
>  103:	for (i=0; i<c->nr_blocks; i++) {
>  104:		struct jffs2_eraseblock *jeb = &c->blocks[i];
>  105:
>  106:		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
>  107:
>  108:		if (ret < 0)
> END -->
>  109:			return ret;
>  110:
>  111:		ACCT_PARANOIA_CHECK(jeb);
>  112:
>  113:		/* Now decide which list to put it on */
>  114:		switch(ret) {

Valid.  And not trivial to fix.

> [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> [FUNC:  jffs2_scan_medium]
> [LINES: 98-233]
> [VAR:   flashbuf]
>   93:			buf_size = c->sector_size;
>   94:		else
>   95:			buf_size = PAGE_SIZE;
>   96:
>   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> START -->
>   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
>   99:		if (!flashbuf)
>  100:			return -ENOMEM;
>  101:	}
>  102:
>  103:	for (i=0; i<c->nr_blocks; i++) {
>         ... DELETED 124 lines ...
>  228:	}
>  229:	if (c->nr_erasing_blocks) {
>  230:		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
>  231:			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
>  232:			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
> END -->
>  233:			return -EIO;
>  234:		}
>  235:		jffs2_erase_pending_trigger(c);
>  236:	}
>  237:	if (buf_size)
>  238:		kfree(flashbuf);

Same one, basically.

Jörn

-- 
Geld macht nicht glücklich.
Glück macht nicht satt.

[PATCH] fix memleak in fs/jffs2/scan.c (was: re: [CHECKER] 32 Memory Leaks on Error Paths)

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> 
> > [FILE:  2.6.0-test5/fs/jffs2/scan.c]
> > [FUNC:  jffs2_scan_medium]
> > [LINES: 98-109]
> > [VAR:   flashbuf]
> >   93:			buf_size = c->sector_size;
> >   94:		else
> >   95:			buf_size = PAGE_SIZE;
> >   96:
> >   97:		D1(printk(KERN_DEBUG "Allocating readbuf of %d bytes\n", buf_size));
> > START -->
> >   98:		flashbuf = kmalloc(buf_size, GFP_KERNEL);
> >   99:		if (!flashbuf)
> >  100:			return -ENOMEM;
> >  101:	}
> >  102:
> >  103:	for (i=0; i<c->nr_blocks; i++) {
> >  104:		struct jffs2_eraseblock *jeb = &c->blocks[i];
> >  105:
> >  106:		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
> >  107:
> >  108:		if (ret < 0)
> > END -->
> >  109:			return ret;
> >  110:
> >  111:		ACCT_PARANOIA_CHECK(jeb);
> >  112:
> >  113:		/* Now decide which list to put it on */
> >  114:		switch(ret) {
> 
> Valid.  And not trivial to fix.

But at least trivial to band-aid around it.  This doesn't make the
function any nicer, but it should get rid of the leaks.

David, consider this to be public domain. :)

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/fs/jffs2/scan.c~jffs2_memleak	2003-07-05 23:59:33.000000000 +0200
+++ linux-2.6.0-test3/fs/jffs2/scan.c	2003-09-16 09:16:30.000000000 +0200
@@ -106,7 +106,7 @@
 		ret = jffs2_scan_eraseblock(c, jeb, buf_size?flashbuf:(flashbuf+jeb->offset), buf_size);
 
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		ACCT_PARANOIA_CHECK(jeb);
 
@@ -230,7 +230,8 @@
 		if ( !c->used_size && ((empty_blocks+bad_blocks)!= c->nr_blocks || bad_blocks == c->nr_blocks) ) {
 			printk(KERN_NOTICE "Cowardly refusing to erase blocks on filesystem with no valid JFFS2 nodes\n");
 			printk(KERN_NOTICE "empty_blocks %d, bad_blocks %d, c->nr_blocks %d\n",empty_blocks,bad_blocks,c->nr_blocks);
-			return -EIO;
+			ret = -EIO;
+			goto out;
 		}
 		jffs2_erase_pending_trigger(c);
 	}
@@ -241,6 +242,10 @@
 		c->mtd->unpoint(c->mtd, flashbuf, 0, c->mtd->size);
 #endif
 	return 0;
+out:
+	if (buf_size)
+		kfree(flashbuf);
+	return ret;
 }
 
 static int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> > 
> > [FILE:  2.6.0-test5/drivers/mtd/chips/cfi_cmdset_0020.c]
> > [FUNC:  cfi_staa_setup]
> > [LINES: 191-211]
> > [VAR:   mtd]
> >  186:	struct mtd_info *mtd;
> >  187:	unsigned long offset = 0;
> >  188:	int i,j;
> >  189:	unsigned long devsize = (1<<cfi->cfiq->DevSize) * cfi->interleave;
> >  190:
> > START -->
> >  191:	mtd = kmalloc(sizeof(*mtd), GFP_KERNEL);
> >  192:	//printk(KERN_DEBUG "number of CFI chips: %d\n", cfi->numchips);
> >  193:
> >  194:	if (!mtd) {
> >  195:		printk(KERN_ERR "Failed to allocate memory for MTD device\n");
> >  196:		kfree(cfi->cmdset_priv);
> >         ... DELETED 9 lines ...
> >  206:	mtd->eraseregions = kmalloc(sizeof(struct mtd_erase_region_info) 
> >  207:			* mtd->numeraseregions, GFP_KERNEL);
> >  208:	if (!mtd->eraseregions) { 
> >  209:		printk(KERN_ERR "Failed to allocate memory for MTD erase region info\n");
> >  210:		kfree(cfi->cmdset_priv);
> > END -->
> >  211:		return NULL;
> >  212:	}
> >  213:	
> >  214:	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
> >  215:		unsigned long ernum, ersize;
> >  216:		ersize = ((cfi->cfiq->EraseRegionInfo[i] >> 8) & ~0xff) * cfi->interleave;
> 
> Valid.

This should be fixed by finally taking the time and merging the
command sets 0001 and 0020.  Maybe someone who cares will come up with
a patch.

Jörn

-- 
Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Wade]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

David Yu Chen wrote:
> Hi All,
> 
> I'm with the Stanford Meta-level Compilation research group, and I
> have a set of memory leaks on error paths for the 2.6.0-test5 kernel.
> (I also have error reports for 2.4.18 and a couple other kernels if
> anyone is interested).
> 
> There may be one or more "GOTO -->" markers showing the different
> paths of execution that can occur between where the memory is
> allocated and where the function returns.
> 
> My checker identifies error paths with a learning algorithm on
> features surrounding goto and return statements.  I'd greatly
> appreciate any comments or confirmation on these bugs.
> 
> Thanks!
> 
> ---
> David Yu Chen
> http://www.stanford.edu/~dychen/
[snip]
> 
> [FILE:  2.6.0-test5/drivers/char/vt_ioctl.c]
> [FUNC:  do_kdsk_ioctl]
> [LINES: 133-150]
> [VAR:   key_map]
>  128:
>  129:			if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
>  130:			    !capable(CAP_SYS_RESOURCE))
>  131:				return -EPERM;
>  132:
> START -->
>  133:			key_map = (ushort *) kmalloc(sizeof(plain_map),
>  134:						     GFP_KERNEL);
>  135:			if (!key_map)
>  136:				return -ENOMEM;
>  137:			key_maps[s] = key_map;
>  138:			key_map[0] = U(K_ALLOCATED);
>         ... DELETED 6 lines ...
>  145:			break;	/* nothing to do */
>  146:		/*
>  147:		 * Attention Key.
>  148:		 */
>  149:		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
> END -->
>  150:			return -EPERM;
>  151:		key_map[i] = U(v);
>  152:		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
>  153:			compute_shiftstate();
>  154:		break;
>  155:	}
> ---------------------------------------------------------
> 

Is the attached correct?



[-- Attachment #2: vt_ioctl_memleak.diff --]
[-- Type: text/plain, Size: 515 bytes --]

--- linux-2.6.0-test5.old/drivers/char/vt_ioctl.c	2003-08-23 07:57:57.000000000 +0800
+++ linux-2.6.0-test5.new/drivers/char/vt_ioctl.c	2003-09-16 16:17:00.000000000 +0800
@@ -146,8 +146,10 @@
 		/*
 		 * Attention Key.
 		 */
-		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
+		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN)) {
+			kfree(key_map);
 			return -EPERM;
+		}
 		key_map[i] = U(v);
 		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
 			compute_shiftstate();

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Tue, 16 September 2003 08:55:53 +0200, Jörn Engel wrote:
> On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> > 
> > looks like checking for mtdblks instead of mtdblk
> > [FILE:  2.6.0-test5/drivers/mtd/mtdblock.c]
> > [FUNC:  mtdblock_open]
> > [LINES: 277-279]
> > [VAR:   mtdblk]
> >  272:		mtdblks[dev]->count++;
> >  273:		return 0;
> >  274:	}
> >  275:	
> >  276:	/* OK, it's not open. Create cache info for it */
> > START -->
> >  277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
> >  278:	if (!mtdblks)
> > END -->
> >  279:		return -ENOMEM;
> >  280:
> >  281:	memset(mtdblk, 0, sizeof(*mtdblk));
> >  282:	mtdblk->count = 1;
> >  283:	mtdblk->mtd = mtd;
> >  284:
> 
> Invalid.  This is quite an obvious false positive, at least if your
> algorithm checks for possible value ranges.

Actually, it *is* valid, as Wade pointed out to me.

David, please apply!

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

--- linux-2.6.0-test3/drivers/mtd/mtdblock.c~mtdblock_leak	2003-07-05 23:59:30.000000000 +0200
+++ linux-2.6.0-test3/drivers/mtd/mtdblock.c	2003-09-16 10:47:58.000000000 +0200
@@ -275,7 +275,7 @@
 	
 	/* OK, it's not open. Create cache info for it */
 	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
-	if (!mtdblks)
+	if (!mtdblk)
 		return -ENOMEM;
 
 	memset(mtdblk, 0, sizeof(*mtdblk));

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Tue, 16 September 2003 16:45:42 +0800, Wade wrote:
> David Yu Chen wrote:
> >
> >[FILE:  2.6.0-test5/drivers/char/vt_ioctl.c]
> >[FUNC:  do_kdsk_ioctl]
> >[LINES: 133-150]
> >[VAR:   key_map]
> > 128:
> > 129:			if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
> > 130:			    !capable(CAP_SYS_RESOURCE))
> > 131:				return -EPERM;
> > 132:
> >START -->
> > 133:			key_map = (ushort *) kmalloc(sizeof(plain_map),
> > 134:						     GFP_KERNEL);
> > 135:			if (!key_map)
> > 136:				return -ENOMEM;
> > 137:			key_maps[s] = key_map;
                                ^^^^^^^^^^^^^^^^^^^^^^
> > 138:			key_map[0] = U(K_ALLOCATED);
> >        ... DELETED 6 lines ...
> > 145:			break;	/* nothing to do */
> > 146:		/*
> > 147:		 * Attention Key.
> > 148:		 */
> > 149:		if (((ov == K_SAK) || (v == K_SAK)) && 
> > !capable(CAP_SYS_ADMIN))
> >END -->
> > 150:			return -EPERM;
> > 151:		key_map[i] = U(v);
> > 152:		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
> > 153:			compute_shiftstate();
> > 154:		break;
> > 155:	}
> >---------------------------------------------------------
> 
> Is the attached correct?

Looks like there is no leak after all, as long as key_maps is dealt
with properly sometime.  But I'm not familiar enough with the code to
judge that.

> --- linux-2.6.0-test5.old/drivers/char/vt_ioctl.c	2003-08-23 07:57:57.000000000 +0800
> +++ linux-2.6.0-test5.new/drivers/char/vt_ioctl.c	2003-09-16 16:17:00.000000000 +0800
> @@ -146,8 +146,10 @@
>  		/*
>  		 * Attention Key.
>  		 */
> -		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
> +		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN)) {
> +			kfree(key_map);
>  			return -EPERM;
> +		}
>  		key_map[i] = U(v);
>  		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
>  			compute_shiftstate();


Jörn

-- 
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jörn Engel]

On Mon, 15 September 2003 21:35:46 -0700, David Yu Chen wrote:
> 
> [FILE:  2.6.0-test5/net/ipv4/igmp.c]
> [FUNC:  igmpv3_newpack]
> [LINES: 274-284]
> [VAR:   skb]
>  269:	struct sk_buff *skb;
>  270:	struct rtable *rt;
>  271:	struct iphdr *pip;
>  272:	struct igmpv3_report *pig;
>  273:
> START -->
>  274:	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
>  275:	if (skb == NULL)
>  276:		return 0;
>  277:
>  278:	{
>  279:		struct flowi fl = { .oif = dev->ifindex,
>  280:				    .nl_u = { .ip4_u = {
>  281:				    .daddr = IGMPV3_ALL_MCR } },
>  282:				    .proto = IPPROTO_IGMP };
>  283:		if (ip_route_output_key(&rt, &fl))
> END -->
>  284:			return 0;
>  285:	}
>  286:	if (rt->rt_src == 0) {
>  287:		ip_rt_put(rt);
>  288:		return 0;
>  289:	}

Looks valid.  And since skb isn't really needed until after these
returns, moving four lines down a bit fixes the problem.

Davem, is this correct?

Jörn

-- 
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

--- linux-2.6.0-test3/net/ipv4/igmp.c~igmp_memleak	2003-07-15 23:09:02.000000000 +0200
+++ linux-2.6.0-test3/net/ipv4/igmp.c	2003-09-16 10:29:47.000000000 +0200
@@ -70,6 +70,8 @@
  *		Alexey Kuznetsov:	Accordance to igmp-v2-06 draft.
  *		David L Stevens:	IGMPv3 support, with help from
  *					Vinay Kulkarni
+ *		Jörn Engel:		Fix memleak in igmpv3_newpack, reported
+ *					by David Yu Chen (stanford checker)
  */
 
 
@@ -271,10 +273,6 @@
 	struct iphdr *pip;
 	struct igmpv3_report *pig;
 
-	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
-	if (skb == NULL)
-		return 0;
-
 	{
 		struct flowi fl = { .oif = dev->ifindex,
 				    .nl_u = { .ip4_u = {
@@ -288,6 +286,10 @@
 		return 0;
 	}
 
+	skb = alloc_skb(size + dev->hard_header_len + 15, GFP_ATOMIC);
+	if (skb == NULL)
+		return 0;
+
 	skb->dst = &rt->u.dst;
 	skb->dev = dev;
 

[PATCH] bttv-risc.c (was: Re: [CHECKER] 32 Memory Leaks on Error Paths)

[Wade]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

David Yu Chen wrote:
> Hi All,
[snip]
> ---------------------------------------------------------
> 
> [FILE:  2.6.0-test5/drivers/media/video/bttv-risc.c]
> [FUNC:  bttv_risc_overlay]
> [LINES: 214-223]
> [VAR:   skips]
>  209:	struct btcx_skiplist *skips;
>  210:	u32 *rp,ri,ra;
>  211:	u32 addr;
>  212:
>  213:	/* skip list for window clipping */
> START -->
>  214:	if (NULL == (skips = kmalloc(sizeof(*skips) * ov->nclips,GFP_KERNEL)))
>  215:		return -ENOMEM;
>  216:	
>  217:	/* estimate risc mem: worst case is (clip+1) * lines instructions
>  218:	   + sync + jump (all 2 dwords) */
>  219:	instructions  = (ov->nclips + 1) *
>  220:		((skip_even || skip_odd) ? ov->w.height>>1 :  ov->w.height);
>  221:	instructions += 2;
>  222:	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0)
> END -->
>  223:		return rc;
>  224:
>  225:	/* sync instruction */
>  226:	rp = risc->cpu;
>  227:	*(rp++) = cpu_to_le32(BT848_RISC_SYNC|BT848_FIFO_STATUS_FM1);
>  228:	*(rp++) = cpu_to_le32(0);
> ---------------------------------------------------------
> 

Correct?



[-- Attachment #2: bttv-risc_memleak.diff --]
[-- Type: text/plain, Size: 547 bytes --]

--- linux-2.6.0-test5.old/drivers/media/video/bttv-risc.c	2003-08-23 07:55:44.000000000 +0800
+++ linux-2.6.0-test5.new/drivers/media/video/bttv-risc.c	2003-09-16 16:48:37.000000000 +0800
@@ -219,8 +219,10 @@
 	instructions  = (ov->nclips + 1) *
 		((skip_even || skip_odd) ? ov->w.height>>1 :  ov->w.height);
 	instructions += 2;
-	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0)
+	if ((rc = btcx_riscmem_alloc(btv->dev,risc,instructions*8)) < 0) {
+		kfree(skips);
 		return rc;
+	}
 
 	/* sync instruction */
 	rp = risc->cpu;

[PATCH] fix memleak in emu10k1/midi.c (was: Re: [CHECKER] 32 Memory Leaks on Error Paths)

[Wade]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

David Yu Chen wrote:
> Hi All,
> 
[snip]
> [FILE:  2.6.0-test5/sound/oss/emu10k1/midi.c]
> [FUNC:  emu10k1_seq_midi_open]
> [LINES: 498-514]
> [VAR:   midi_dev]
>  493:	if (card->open_mode)		/* card is opened native */
>  494:		return -EBUSY;
>  495:			
>  496:	DPF(2, "emu10k1_seq_midi_open()\n");
>  497:	
> START -->
>  498:	if ((midi_dev = (struct emu10k1_mididevice *) kmalloc(sizeof(*midi_dev), GFP_KERNEL)) == NULL)
>  499:		return -EINVAL;
>  500:
>  501:	midi_dev->card = card;
>  502:	midi_dev->mistate = MIDIIN_STATE_STOPPED;
>  503:	init_waitqueue_head(&midi_dev->oWait);
>         ... DELETED 5 lines ...
>  509:
>  510:	dsCardMidiOpenInfo.refdata = (unsigned long) midi_dev;
>  511:
>  512:	if (emu10k1_mpuout_open(card, &dsCardMidiOpenInfo) < 0) {
>  513:		ERROR();
> END -->
>  514:		return -ENODEV;
>  515:	}
>  516:
>  517:	card->seq_mididev = midi_dev;
>  518:		
>  519:	return 0;
> ---------------------------------------------------------


Can I ask again why the Checker is not released? I know that you don't 
have to release the source since you are not distributing it, but why 
not? AFAICS this would be a great asset to Open Source projects in 
general, not just the kernel.

(Patch correct? looked obvious)




[-- Attachment #2: emu10k1_midi_memleak.diff --]
[-- Type: text/plain, Size: 313 bytes --]

--- linux-2.6.0-test5.old/sound/oss/emu10k1/midi.c	2003-09-09 21:24:39.000000000 +0800
+++ linux-2.6.0-test5.new/sound/oss/emu10k1/midi.c	2003-09-16 18:08:23.000000000 +0800
@@ -511,6 +511,7 @@
 
 	if (emu10k1_mpuout_open(card, &dsCardMidiOpenInfo) < 0) {
 		ERROR();
+		kfree(midi_dev);
 		return -ENODEV;
 	}
 

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Andries Brouwer]

On Mon, Sep 15, 2003 at 09:35:46PM -0700, David Yu Chen wrote:

> I'm with the Stanford Meta-level Compilation research group, and I
> have a set of memory leaks on error paths for the 2.6.0-test5 kernel.

There is no memory leak in the below:

> ---------------------------------------------------------
> 
> [FILE:  2.6.0-test5/drivers/char/vt_ioctl.c]
> [FUNC:  do_kdsk_ioctl]
> [LINES: 133-150]
> [VAR:   key_map]
>  128:
>  129:			if (keymap_count >= MAX_NR_OF_USER_KEYMAPS &&
>  130:			    !capable(CAP_SYS_RESOURCE))
>  131:				return -EPERM;
>  132:
> START -->
>  133:			key_map = (ushort *) kmalloc(sizeof(plain_map),
>  134:						     GFP_KERNEL);
>  135:			if (!key_map)
>  136:				return -ENOMEM;
>  137:			key_maps[s] = key_map;
>  138:			key_map[0] = U(K_ALLOCATED);
>         ... DELETED 6 lines ...
>  145:			break;	/* nothing to do */
>  146:		/*
>  147:		 * Attention Key.
>  148:		 */
>  149:		if (((ov == K_SAK) || (v == K_SAK)) && !capable(CAP_SYS_ADMIN))
> END -->
>  150:			return -EPERM;
>  151:		key_map[i] = U(v);
>  152:		if (!s && (KTYP(ov) == KT_SHIFT || KTYP(v) == KT_SHIFT))
>  153:			compute_shiftstate();
>  154:		break;
>  155:	}

Explanation:
The user may have up to 256 keymaps, namely the plain map and
various maps for (combinations of) modifier keys.
In order to save memory, most of these keymaps will not be allocated.
If one sets an entry on a keymap, then first the keymap itself is
allocated in case it did not exist yet. Then the entry is filled,
assuming one has permission.
On the error path the memory is not lost: the pointer was stored
in the array key_maps[], so also am automatic checker could see that.
(And indeed, it is also possible to deallocate keymaps.)

Andries

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Andries Brouwer]

On Tue, Sep 16, 2003 at 04:45:42PM +0800, Wade wrote:

> Is the attached correct?

No, it frees memory that is still in use, and will oops the kernel.

> --- linux-2.6.0-test5.old/drivers/char/vt_ioctl.c	2003-08-23 07:57:57.000000000 +0800

There was no bug to start with, so please do not fix anything.

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Timothy Miller]

>> 276:	/* OK, it's not open. Create cache info for it */
>>START -->
>> 277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>> 278:	if (!mtdblks)
>>END -->
>> 279:		return -ENOMEM;

> 
> Invalid.  This is quite an obvious false positive, at least if your
> algorithm checks for possible value ranges.

Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
's' on it.  Unless there is some kind of aliasing going on, they would 
appear to be different variables.  Naturally, I didn't check the 
original code, so I could be full of it.  :)

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Wade]

Timothy Miller wrote:
> 
>>> 276:    /* OK, it's not open. Create cache info for it */
>>> START -->
>>> 277:    mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>>> 278:    if (!mtdblks)
>>> END -->
>>> 279:        return -ENOMEM;
> 
> 
>>
>> Invalid.  This is quite an obvious false positive, at least if your
>> algorithm checks for possible value ranges.
> 
> 
> Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
> 's' on it.  Unless there is some kind of aliasing going on, they would 
> appear to be different variables.  Naturally, I didn't check the 
> original code, so I could be full of it.  :)
> 
This has been resolved, see Jorn's other mail.

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Tue, 16 Sep 2003 10:52:06 EDT, Timothy Miller said:
> 
> >> 276:	/* OK, it's not open. Create cache info for it */
> >>START -->
> >> 277:	mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
> >> 278:	if (!mtdblks)
> >>END -->
> >> 279:		return -ENOMEM;
> 
> > 
> > Invalid.  This is quite an obvious false positive, at least if your
> > algorithm checks for possible value ranges.
> 
> Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
> 's' on it.  Unless there is some kind of aliasing going on, they would 
> appear to be different variables.  Naturally, I didn't check the 
> original code, so I could be full of it.  :)

On the other hand, even if the report was invalid (which another posting says isn't
the case), this code is just *begging* for somebody to "fix the typo", due to how
much the kernel uses
	foo = function(..)
	if (!foo)
Yeah, they're different variables - but we alloc mktblk and then return -ENOMEM
if the OTHER variable is null?? ;)

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Nick Piggin]

Timothy Miller wrote:

>
>>> 276:    /* OK, it's not open. Create cache info for it */
>>> START -->
>>> 277:    mtdblk = kmalloc(sizeof(struct mtdblk_dev), GFP_KERNEL);
>>> 278:    if (!mtdblks)
>>> END -->
>>> 279:        return -ENOMEM;
>>
>
>>
>> Invalid.  This is quite an obvious false positive, at least if your
>> algorithm checks for possible value ranges.
>
>
> Wait... one is "mtdblk", and the other is "mtdblks".  One has an extra 
> 's' on it.  Unless there is some kind of aliasing going on, they would 
> appear to be different variables.  Naturally, I didn't check the 
> original code, so I could be full of it.  :)


Yes its a bug from glancing at the source code.

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/security/selinux/selinuxfs.c]
> START -->
>  253:	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
>  254:	if (!ar)
>  255:		return -ENOMEM;
>  256:	memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
>  257:	ar->size = 0;
>  258:	down(&file->f_dentry->d_inode->i_sem);
>         ... DELETED 5 lines ...

A reference to ar is stored away for kfree() on ->release():
		file->private_data = ar;

>  268:	if (copy_from_user(ar->data, buf, size))
> END -->
>  269:		return -EFAULT;

So, this will be freed when the file is closed.  I don't think it's a
bug.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/security/selinux/ss/mls.c]
> [VAR:   r]
>  538:		goto out;
>  539:	}
>  540:	nel = le32_to_cpu(buf[0]);
>  541:	l = NULL;
>  542:	for (i = 0; i < nel; i++) {
> START -->
>  543:		r = kmalloc(sizeof(*r), GFP_ATOMIC);
>  544:		if (!r) {
>  545:			rc = -ENOMEM;
>  546:			goto out;
>  547:		}
>  548:		memset(r, 0, sizeof(*r));
>  549:
>  550:		rc = mls_read_range_helper(&r->range, fp);
>  551:		if (rc)
> GOTO -->
>  552:			goto out;

Yes, this looks like a bug.  Stephen, does this fix look ok?
thanks
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net


===== security/selinux/ss/mls.c 1.1 vs edited =====
--- 1.1/security/selinux/ss/mls.c	Thu Jul 17 02:38:01 2003
+++ edited/security/selinux/ss/mls.c	Fri Sep 19 13:56:50 2003
@@ -548,8 +548,10 @@
 		memset(r, 0, sizeof(*r));
 
 		rc = mls_read_range_helper(&r->range, fp);
-		if (rc)
+		if (rc) {
+			kfree(r);
 			goto out;
+		}
 
 		if (l)
 			l->next = r;

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 1232:			c = kmalloc(sizeof(*c), GFP_KERNEL);
> 1233:			if (!c) {
> 1234:				rc = -ENOMEM;
> 1235:				goto bad;
> 1236:			}
> 1237:			memset(c, 0, sizeof(*c));
>         ... DELETED 182 lines ...

You missed a reference to the context is stored in the policydb:
			p->ocontexts[i] = c;
> END -->
> 1427:	return rc;
> 1428:bad:
> 1429:	policydb_destroy(p);

The policydb is destroyed on error, which will free the context.  I
don't think this is a bug.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 1334:		newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
> 1335:		if (!newgenfs) {
> 1336:			rc = -ENOMEM;
> 1337:			goto bad;
> 1338:		}
> 1339:		memset(newgenfs, 0, sizeof(*newgenfs));
>         ... DELETED 5 lines ...
You missed one ;-)

1344		buf = next_entry(fp, len);
1345		if (!buf)
1346			goto bad;


> GOTO -->
> 1350:			goto bad;

Yes, this is a bug.

> 1357:				printk(KERN_ERR "security:  dup genfs "
> 1358:				       "fstype %s\n", newgenfs->fstype);
> GOTO -->
> 1359:				goto bad;

Yes, this is a bug.  I'm not sure if CHECKER caught that it leaks two
allocations here.  Both newgenfs and newgenfs->fstype.

> [FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
> 1374:			newc = kmalloc(sizeof(*newc), GFP_KERNEL);
> 1375:			if (!newc) {
> 1376:				rc = -ENOMEM;
> 1377:				goto bad;
> 1378:			}
> 1379:			memset(newc, 0, sizeof(*newc));

You missed some more.  Not sure if this is meant to be comprensive or not...

1380			buf = next_entry(fp, sizeof(u32));
1381			if (!buf)
1382				goto bad;
*HERE*
1383			len = le32_to_cpu(buf[0]);
1384			buf = next_entry(fp, len);
1385			if (!buf)
1386				goto bad;
*HERE*
1387			 newc->u.name = kmalloc(len + 1,GFP_KERNEL);
1388			 if (!newc->u.name) {
1389				 rc = -ENOMEM;
1390				 goto bad;
*HERE*

>         ... DELETED 14 lines ...
> 1394:			buf = next_entry(fp, sizeof(u32));
> 1395:			if (!buf)
> 1396:				goto bad;

Again, not sure if you meant to capture both leakages here: newc and
newc->u.name.

> 1397:			newc->v.sclass = le32_to_cpu(buf[0]);
> 1398:			if (context_read_and_validate(&newc->context[0], p, fp))
> GOTO -->
> 1399:				goto bad;

And both here.

> 1400:			for (l = NULL, c = newgenfs->head; c;
> 1401:			     l = c, c = c->next) {
> 1402:				if (!strcmp(newc->u.name, c->u.name) &&
> 1403:				    (!c->v.sclass || !newc->v.sclass ||
> 1404:				     newc->v.sclass == c->v.sclass)) {
> 1405:					printk(KERN_ERR "security:  dup genfs "
> 1406:					       "entry (%s,%s)\n",
> 1407:					       newgenfs->fstype, c->u.name);
> GOTO -->
> 1408:					goto bad;

And both here.

Patch below fixes the leaks in a brute force method.  Stephen, look ok?
I could see rearranging the code to ensure things are stored in the policydb
earlier so they are all caught by policydb_destroy().

> [FILE:  2.6.0-test5/security/selinux/ss/policydb.c]
> START -->
>  759:		c = kmalloc(sizeof(*c), GFP_KERNEL);
>  760:		if (!c) {
>  761:			rc = -ENOMEM;
>  762:			goto bad;
>  763:		}
>  764:		memset(c, 0, sizeof(*c));

You missed this one:
765		 buf = next_entry(fp, sizeof(u32)*2);
766		 if (!buf)
767			 goto bad;
And this one:
774			 if (!e) {
775				 rc = -ENOMEM;
776				 goto bad;
And this one:
780			 if (!buf) {
781				 kfree(e);
782				 goto bad;
And this one:
790				 if (depth < 0) {
791					 kfree(e);
792					 goto bad;
etc...

>         ... DELETED 64 lines ...
>  829:				c->expr = e;
>  830:			}
>  831:			le = e;
>  832:		}
>  833:		if (depth != 0)
> GOTO -->

Yes, this is a bug.  Stephen, the patch below fixes this, and adds a
constraint_destroy() function to help cleanup when the constaint_node
isn't yet attached to the class_datum but there may already be some
constraint_expr's on the constraint_node.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net


===== security/selinux/ss/policydb.c 1.3 vs edited =====
--- 1.3/security/selinux/ss/policydb.c	Sun Aug 31 16:14:19 2003
+++ edited/security/selinux/ss/policydb.c	Fri Sep 19 15:59:02 2003
@@ -321,17 +321,11 @@
 	return 0;
 }
 
-static int class_destroy(void *key, void *datum, void *p)
+static void constraint_destroy(struct constraint_node *constraint)
 {
-	struct class_datum *cladatum;
-	struct constraint_node *constraint, *ctemp;
+	struct constraint_node *ctemp;
 	struct constraint_expr *e, *etmp;
 
-	kfree(key);
-	cladatum = datum;
-	hashtab_map(cladatum->permissions.table, perm_destroy, 0);
-	hashtab_destroy(cladatum->permissions.table);
-	constraint = cladatum->constraints;
 	while (constraint) {
 		e = constraint->expr;
 		while (e) {
@@ -344,6 +338,19 @@
 		constraint = constraint->next;
 		kfree(ctemp);
 	}
+}
+
+static int class_destroy(void *key, void *datum, void *p)
+{
+	struct class_datum *cladatum;
+	struct constraint_node *constraint;
+
+	kfree(key);
+	cladatum = datum;
+	hashtab_map(cladatum->permissions.table, perm_destroy, 0);
+	hashtab_destroy(cladatum->permissions.table);
+	constraint = cladatum->constraints;
+	constraint_destroy(constraint);
 	kfree(cladatum->comkey);
 	kfree(datum);
 	return 0;
@@ -763,8 +770,10 @@
 		}
 		memset(c, 0, sizeof(*c));
 		buf = next_entry(fp, sizeof(u32)*2);
-		if (!buf)
+		if (!buf) {
+			kfree(c);
 			goto bad;
+		}
 		c->permissions = le32_to_cpu(buf[0]);
 		nexpr = le32_to_cpu(buf[1]);
 		le = NULL;
@@ -773,11 +782,13 @@
 			e = kmalloc(sizeof(*e), GFP_KERNEL);
 			if (!e) {
 				rc = -ENOMEM;
+				constraint_destroy(c);
 				goto bad;
 			}
 			memset(e, 0, sizeof(*e));
 			buf = next_entry(fp, sizeof(u32)*3);
 			if (!buf) {
+				constraint_destroy(c);
 				kfree(e);
 				goto bad;
 			}
@@ -788,6 +799,7 @@
 			switch (e->expr_type) {
 			case CEXPR_NOT:
 				if (depth < 0) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -795,6 +807,7 @@
 			case CEXPR_AND:
 			case CEXPR_OR:
 				if (depth < 1) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -802,6 +815,7 @@
 				break;
 			case CEXPR_ATTR:
 				if (depth == (CEXPR_MAXDEPTH-1)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
@@ -809,16 +823,19 @@
 				break;
 			case CEXPR_NAMES:
 				if (depth == (CEXPR_MAXDEPTH-1)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
 				depth++;
 				if (ebitmap_read(&e->names, fp)) {
+					constraint_destroy(c);
 					kfree(e);
 					goto bad;
 				}
 				break;
 			default:
+				constraint_destroy(c);
 				kfree(e);
 				goto bad;
 				break;
@@ -830,8 +847,10 @@
 			}
 			le = e;
 		}
-		if (depth != 0)
+		if (depth != 0) {
+			constraint_destroy(c);
 			goto bad;
+		}
 		if (lc) {
 			lc->next = c;
 		} else {
@@ -1338,15 +1357,20 @@
 		}
 		memset(newgenfs, 0, sizeof(*newgenfs));
 		buf = next_entry(fp, sizeof(u32));
-		if (!buf)
+		if (!buf) {
+			kfree(newgenfs);
 			goto bad;
+		}
 		len = le32_to_cpu(buf[0]);
 		buf = next_entry(fp, len);
-		if (!buf)
+		if (!buf) {
+			kfree(newgenfs);
 			goto bad;
+		}
 		newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
 		if (!newgenfs->fstype) {
 			rc = -ENOMEM;
+			kfree(newgenfs);
 			goto bad;
 		}
 		memcpy(newgenfs->fstype, buf, len);
@@ -1356,6 +1380,8 @@
 			if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
 				printk(KERN_ERR "security:  dup genfs "
 				       "fstype %s\n", newgenfs->fstype);
+				kfree(newgenfs->fstype);
+				kfree(newgenfs);
 				goto bad;
 			}
 			if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
@@ -1378,25 +1404,36 @@
 			}
 			memset(newc, 0, sizeof(*newc));
 			buf = next_entry(fp, sizeof(u32));
-			if (!buf)
+			if (!buf) {
+				kfree(newc);
 				goto bad;
+			}
 			len = le32_to_cpu(buf[0]);
 			buf = next_entry(fp, len);
-			if (!buf)
+			if (!buf) {
+				kfree(newc);
 				goto bad;
+			}
 			newc->u.name = kmalloc(len + 1,GFP_KERNEL);
 			if (!newc->u.name) {
 				rc = -ENOMEM;
+				kfree(newc);
 				goto bad;
 			}
 			memcpy(newc->u.name, buf, len);
 			newc->u.name[len] = 0;
 			buf = next_entry(fp, sizeof(u32));
-			if (!buf)
+			if (!buf) {
+				kfree(newc->u.name);
+				kfree(newc);
 				goto bad;
+			}
 			newc->v.sclass = le32_to_cpu(buf[0]);
-			if (context_read_and_validate(&newc->context[0], p, fp))
+			if (context_read_and_validate(&newc->context[0], p, fp)){ 
+				kfree(newc->u.name);
+				kfree(newc);
 				goto bad;
+			}
 			for (l = NULL, c = newgenfs->head; c;
 			     l = c, c = c->next) {
 				if (!strcmp(newc->u.name, c->u.name) &&
@@ -1405,6 +1442,8 @@
 					printk(KERN_ERR "security:  dup genfs "
 					       "entry (%s,%s)\n",
 					       newgenfs->fstype, c->u.name);
+					kfree(newc->u.name);
+					kfree(newc);
 					goto bad;
 				}
 				len = strlen(newc->u.name);

Re: [CHECKER] 32 Memory Leaks on Error Paths

[David S. Miller]

On Tue, 16 Sep 2003 11:07:48 +0200
Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:

> Looks valid.  And since skb isn't really needed until after these
> returns, moving four lines down a bit fixes the problem.
> 
> Davem, is this correct?

Nope, now you're leaking the route instead of the SKB :-)

I'll put the correct fix in.

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/drivers/scsi/NCR_Q720.c]
> START -->
>  153:	p = kmalloc(sizeof(*p), GFP_KERNEL);
>  154:	if (!p)
>  155:		return -ENOMEM;
<snip>
>  180:	if(i != NCR_Q720_MCA_ID) {
>  181:		printk(KERN_ERR "NCR_Q720, adapter failed to I/O map registers correctly at 0x%x(0x%x)\n", io_base, i);
> END -->
>  182:		return -ENODEV;

Yes, looks like a valid bug.  Patch below.  James, look ok?
thanks,
-chris

===== drivers/scsi/NCR_Q720.c 1.4 vs edited =====
--- 1.4/drivers/scsi/NCR_Q720.c	Sun Aug 17 13:10:45 2003
+++ edited/drivers/scsi/NCR_Q720.c	Mon Sep 22 15:05:02 2003
@@ -179,7 +179,7 @@
 	i = inb(io_base) | (inb(io_base+1)<<8);
 	if(i != NCR_Q720_MCA_ID) {
 		printk(KERN_ERR "NCR_Q720, adapter failed to I/O map registers correctly at 0x%x(0x%x)\n", io_base, i);
-		return -ENODEV;
+		goto out_free;
 	}
 
 	/* Phase II, find the ram base and memory map the board register */

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/drivers/scsi/scsi_debug.c]
> [FUNC:  sdebug_add_adapter]
> [LINES: 1612-1652]
> [VAR:   sdbg_devinfo]
> 1607:        memset(sdbg_host, 0, sizeof(*sdbg_host));
> 1608:        INIT_LIST_HEAD(&sdbg_host->dev_info_list);
> 1609:
> 1610:	devs_per_host = scsi_debug_num_tgts * scsi_debug_max_luns;
> 1611:        for (k = 0; k < devs_per_host; k++) {
> START -->
> 1612:                sdbg_devinfo = kmalloc(sizeof(*sdbg_devinfo),GFP_KERNEL);
> 1613:                if (NULL == sdbg_devinfo) {
> 1614:                        printk(KERN_ERR "%s: out of memory at line %d\n",
> 1615:                               __FUNCTION__, __LINE__);
> 1616:                        error = -ENOMEM;
> GOTO -->
> 1617:			goto clean1;
>         ... DELETED 29 lines ...
> 1647:		kfree(sdbg_devinfo);
> 1648:	}
> 1649:
> 1650:clean1:
> 1651:	kfree(sdbg_host);
> END -->
> 1652:        return error;

Yes, this could leak sdbg_devinfo structs.  Patch below.  James, look ok?

thanks,
-chris

===== drivers/scsi/scsi_debug.c 1.44 vs edited =====
--- 1.44/drivers/scsi/scsi_debug.c	Fri Aug 15 10:07:53 2003
+++ edited/drivers/scsi/scsi_debug.c	Mon Sep 22 15:27:17 2003
@@ -1614,7 +1614,7 @@
                         printk(KERN_ERR "%s: out of memory at line %d\n",
                                __FUNCTION__, __LINE__);
                         error = -ENOMEM;
-			goto clean1;
+			goto clean;
                 }
                 memset(sdbg_devinfo, 0, sizeof(*sdbg_devinfo));
                 sdbg_devinfo->sdbg_host = sdbg_host;
@@ -1634,12 +1634,12 @@
         error = device_register(&sdbg_host->dev);
 
         if (error)
-		goto clean2;
+		goto clean;
 
 	++scsi_debug_add_host;
         return error;
 
-clean2:
+clean:
 	list_for_each_safe(lh, lh_sf, &sdbg_host->dev_info_list) {
 		sdbg_devinfo = list_entry(lh, struct sdebug_dev_info,
 					  dev_list);
@@ -1647,7 +1647,6 @@
 		kfree(sdbg_devinfo);
 	}
 
-clean1:
 	kfree(sdbg_host);
         return error;
 }

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/drivers/telephony/ixj_pcmcia.c]
> [FUNC:  ixj_attach]
> [LINES: 53-64]
> [VAR:   link]
>   48:	client_reg_t client_reg;
>   49:	dev_link_t *link;
>   50:	int ret;
>   51:	DEBUG(0, "ixj_attach()\n");
>   52:	/* Create new ixj device */
> START -->
>   53:	link = kmalloc(sizeof(struct dev_link_t), GFP_KERNEL);
>   54:	if (!link)
>   55:		return NULL;
>   56:	memset(link, 0, sizeof(struct dev_link_t));
>   57:	link->io.Attributes1 = IO_DATA_PATH_WIDTH_8;
>   58:	link->io.Attributes2 = IO_DATA_PATH_WIDTH_8;
>   59:	link->io.IOAddrLines = 3;
>   60:	link->conf.Vcc = 50;
>   61:	link->conf.IntType = INT_MEMORY_AND_IO;
>   62:	link->priv = kmalloc(sizeof(struct ixj_info_t), GFP_KERNEL);
>   63:	if (!link->priv)
> END -->
>   64:		return NULL;

Yes, this is a bug.  Patch below.

thanks,
-chris

===== drivers/telephony/ixj_pcmcia.c 1.6 vs edited =====
--- 1.6/drivers/telephony/ixj_pcmcia.c	Sat Aug 16 13:22:52 2003
+++ edited/drivers/telephony/ixj_pcmcia.c	Mon Sep 22 15:38:40 2003
@@ -60,8 +60,10 @@
 	link->conf.Vcc = 50;
 	link->conf.IntType = INT_MEMORY_AND_IO;
 	link->priv = kmalloc(sizeof(struct ixj_info_t), GFP_KERNEL);
-	if (!link->priv)
+	if (!link->priv) {
+		kfree(link);
 		return NULL;
+	}
 	memset(link->priv, 0, sizeof(struct ixj_info_t));
 	/* Register with Card Services */
 	link->next = dev_list;

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Stephen Smalley]

On Fri, 2003-09-19 at 19:04, Chris Wright wrote:
> Yes, this is a bug.  Stephen, the patch below fixes this, and adds a
> constraint_destroy() function to help cleanup when the constaint_node
> isn't yet attached to the class_datum but there may already be some
> constraint_expr's on the constraint_node.

Hi Chris,

Sorry for the duplicated effort, but I had already written up a patch
prior to the hurricane, but didn't get it sent out.  I believe that the
patch below fixes the legitimate leaks in the SELinux code.  In some
cases, it rearranges the code (moving the allocation later to reduce the
need for further cleanup or linking the object into a containing
structure earlier so that the policydb_destroy will handle it upon any
subsequent errors).

 security/selinux/ss/mls.c      |   13 ++++
 security/selinux/ss/policydb.c |  112 ++++++++++++++++++++---------------------
 2 files changed, 68 insertions(+), 57 deletions(-)

Index: linux-2.6/security/selinux/ss/mls.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/ss/mls.c,v
retrieving revision 1.16
diff -u -r1.16 mls.c
--- linux-2.6/security/selinux/ss/mls.c	17 Jul 2003 11:33:35 -0000	1.16
+++ linux-2.6/security/selinux/ss/mls.c	16 Sep 2003 19:45:35 -0000
@@ -548,8 +548,10 @@
 		memset(r, 0, sizeof(*r));
 
 		rc = mls_read_range_helper(&r->range, fp);
-		if (rc)
+		if (rc) {
+			kfree(r);
 			goto out;
+		}
 
 		if (l)
 			l->next = r;
@@ -581,10 +583,17 @@
 		goto out;
 	rc = ebitmap_read(&p->trustedwriters, fp);
 	if (rc)
-		goto out;
+		goto bad;
 	rc = ebitmap_read(&p->trustedobjects, fp);
+	if (rc)
+		goto bad2;
 out:
 	return rc;
+bad2:
+	ebitmap_destroy(&p->trustedwriters);
+bad:
+	ebitmap_destroy(&p->trustedreaders);
+	goto out;
 }
 
 int sens_index(void *key, void *datum, void *datap)
Index: linux-2.6/security/selinux/ss/policydb.c
===================================================================
RCS file: /nfshome/pal/CVS/linux-2.6/security/selinux/ss/policydb.c,v
retrieving revision 1.24
diff -u -r1.24 policydb.c
--- linux-2.6/security/selinux/ss/policydb.c	26 Aug 2003 13:13:50 -0000	1.24
+++ linux-2.6/security/selinux/ss/policydb.c	16 Sep 2003 19:57:10 -0000
@@ -390,6 +390,16 @@
 	mls_destroy_f
 };
 
+void ocontext_destroy(struct ocontext *c, int i)
+{
+	context_destroy(&c->context[0]);
+	context_destroy(&c->context[1]);
+	if (i == OCON_ISID || i == OCON_FS ||
+	    i == OCON_NETIF || i == OCON_FSUSE)
+		kfree(c->u.name);
+	kfree(c);
+}
+
 /*
  * Free any memory allocated by a policy database structure.
  */
@@ -423,12 +433,7 @@
 		while (c) {
 			ctmp = c;
 			c = c->next;
-			context_destroy(&ctmp->context[0]);
-			context_destroy(&ctmp->context[1]);
-			if (i == OCON_ISID || i == OCON_FS ||
-			    i == OCON_NETIF || i == OCON_FSUSE)
-				kfree(ctmp->u.name);
-			kfree(ctmp);
+			ocontext_destroy(ctmp,i);
 		}
 	}
 
@@ -439,9 +444,7 @@
 		while (c) {
 			ctmp = c;
 			c = c->next;
-			context_destroy(&ctmp->context[0]);
-			kfree(ctmp->u.name);
-			kfree(ctmp);
+			ocontext_destroy(ctmp,OCON_FSUSE);
 		}
 		gtmp = g;
 		g = g->next;
@@ -762,6 +765,13 @@
 			goto bad;
 		}
 		memset(c, 0, sizeof(*c));
+
+		if (lc) {
+			lc->next = c;
+		} else {
+			cladatum->constraints = c;
+		}
+
 		buf = next_entry(fp, sizeof(u32)*2);
 		if (!buf)
 			goto bad;
@@ -776,67 +786,50 @@
 				goto bad;
 			}
 			memset(e, 0, sizeof(*e));
+
+			if (le) {
+				le->next = e;
+			} else {
+				c->expr = e;
+			}
+
 			buf = next_entry(fp, sizeof(u32)*3);
-			if (!buf) {
-				kfree(e);
+			if (!buf)
 				goto bad;
-			}
 			e->expr_type = le32_to_cpu(buf[0]);
 			e->attr = le32_to_cpu(buf[1]);
 			e->op = le32_to_cpu(buf[2]);
 
 			switch (e->expr_type) {
 			case CEXPR_NOT:
-				if (depth < 0) {
-					kfree(e);
+				if (depth < 0)
 					goto bad;
-				}
 				break;
 			case CEXPR_AND:
 			case CEXPR_OR:
-				if (depth < 1) {
-					kfree(e);
+				if (depth < 1)
 					goto bad;
-				}
 				depth--;
 				break;
 			case CEXPR_ATTR:
-				if (depth == (CEXPR_MAXDEPTH-1)) {
-					kfree(e);
+				if (depth == (CEXPR_MAXDEPTH-1))
 					goto bad;
-				}
 				depth++;
 				break;
 			case CEXPR_NAMES:
-				if (depth == (CEXPR_MAXDEPTH-1)) {
-					kfree(e);
+				if (depth == (CEXPR_MAXDEPTH-1))
 					goto bad;
-				}
 				depth++;
-				if (ebitmap_read(&e->names, fp)) {
-					kfree(e);
+				if (ebitmap_read(&e->names, fp))
 					goto bad;
-				}
 				break;
 			default:
-				kfree(e);
 				goto bad;
-				break;
-			}
-			if (le) {
-				le->next = e;
-			} else {
-				c->expr = e;
 			}
 			le = e;
 		}
 		if (depth != 0)
 			goto bad;
-		if (lc) {
-			lc->next = c;
-		} else {
-			cladatum->constraints = c;
-		}
 		lc = c;
 	}
 
@@ -1331,12 +1324,6 @@
 	genfs_p = NULL;
 	rc = -EINVAL;
 	for (i = 0; i < nel; i++) {
-		newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
-		if (!newgenfs) {
-			rc = -ENOMEM;
-			goto bad;
-		}
-		memset(newgenfs, 0, sizeof(*newgenfs));
 		buf = next_entry(fp, sizeof(u32));
 		if (!buf)
 			goto bad;
@@ -1344,9 +1331,17 @@
 		buf = next_entry(fp, len);
 		if (!buf)
 			goto bad;
+		newgenfs = kmalloc(sizeof(*newgenfs), GFP_KERNEL);
+		if (!newgenfs) {
+			rc = -ENOMEM;
+			goto bad;
+		}
+		memset(newgenfs, 0, sizeof(*newgenfs));
+
 		newgenfs->fstype = kmalloc(len + 1,GFP_KERNEL);
 		if (!newgenfs->fstype) {
 			rc = -ENOMEM;
+			kfree(newgenfs);
 			goto bad;
 		}
 		memcpy(newgenfs->fstype, buf, len);
@@ -1356,6 +1351,8 @@
 			if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
 				printk(KERN_ERR "security:  dup genfs "
 				       "fstype %s\n", newgenfs->fstype);
+				kfree(newgenfs->fstype);
+				kfree(newgenfs);
 				goto bad;
 			}
 			if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
@@ -1371,12 +1368,6 @@
 			goto bad;
 		nel2 = le32_to_cpu(buf[0]);
 		for (j = 0; j < nel2; j++) {
-			newc = kmalloc(sizeof(*newc), GFP_KERNEL);
-			if (!newc) {
-				rc = -ENOMEM;
-				goto bad;
-			}
-			memset(newc, 0, sizeof(*newc));
 			buf = next_entry(fp, sizeof(u32));
 			if (!buf)
 				goto bad;
@@ -1384,19 +1375,27 @@
 			buf = next_entry(fp, len);
 			if (!buf)
 				goto bad;
+
+			newc = kmalloc(sizeof(*newc), GFP_KERNEL);
+			if (!newc) {
+				rc = -ENOMEM;
+				goto bad;
+			}
+			memset(newc, 0, sizeof(*newc));
+
 			newc->u.name = kmalloc(len + 1,GFP_KERNEL);
 			if (!newc->u.name) {
 				rc = -ENOMEM;
-				goto bad;
+				goto bad_newc;
 			}
 			memcpy(newc->u.name, buf, len);
 			newc->u.name[len] = 0;
 			buf = next_entry(fp, sizeof(u32));
 			if (!buf)
-				goto bad;
+				goto bad_newc;
 			newc->v.sclass = le32_to_cpu(buf[0]);
 			if (context_read_and_validate(&newc->context[0], p, fp))
-				goto bad;
+				goto bad_newc;
 			for (l = NULL, c = newgenfs->head; c;
 			     l = c, c = c->next) {
 				if (!strcmp(newc->u.name, c->u.name) &&
@@ -1405,13 +1404,14 @@
 					printk(KERN_ERR "security:  dup genfs "
 					       "entry (%s,%s)\n",
 					       newgenfs->fstype, c->u.name);
-					goto bad;
+					goto bad_newc;
 				}
 				len = strlen(newc->u.name);
 				len2 = strlen(c->u.name);
 				if (len > len2)
 					break;
 			}
+
 			newc->next = c;
 			if (l)
 				l->next = newc;
@@ -1425,6 +1425,8 @@
 		goto bad;
 out:
 	return rc;
+bad_newc:
+	ocontext_destroy(newc,OCON_FSUSE);
 bad:
 	policydb_destroy(p);
 	goto out;


-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* Stephen Smalley (sds@epoch.ncsc.mil) wrote:
> Sorry for the duplicated effort, but I had already written up a patch
> prior to the hurricane, but didn't get it sent out.  I believe that the
> patch below fixes the legitimate leaks in the SELinux code.  In some
> cases, it rearranges the code (moving the allocation later to reduce the
> need for further cleanup or linking the object into a containing
> structure earlier so that the policydb_destroy will handle it upon any
> subsequent errors).

No problem, your patch looks better anyway ;-)
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> Leaks if devices == 0 ?  Error_end only frees mdevs if (devices > 0), 
> but for mdevs=kmalloc(0), the slab allocator may still actually return memory
> [FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
> [FUNC:  alloc_usb_midi_device]
> [LINES: 1621-1772]
> [VAR:   mdevs]
> 1616:	devices = inDevs > outDevs ? inDevs : outDevs;
> 1617:	devices = maxdevices > devices ? devices : maxdevices;
> 1618:
> 1619:	/* obtain space for device name (iProduct) if not known. */
> 1620:	if ( ! u->deviceName ) {
> START -->
> 1621:		mdevs = (struct usb_mididev **)
> 1622:			kmalloc(sizeof(struct usb_mididevs *)*devices
> 1623:				+ sizeof(char) * 256, GFP_KERNEL);
<snip>
> GOTO -->
> 1715:			goto error_end;
<snip>
> END -->
> 1772:	return -ENOMEM;
> [FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
> START -->
> 1625:		mdevs = (struct usb_mididev **)
> 1626:			kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
<snip>
> GOTO -->
> 1715:			goto error_end;
<snip>
> END -->
> 1772:	return -ENOMEM;

Yes, these are bugs.  Patch below.  Greg, this look ok?

thanks,
-chris

===== drivers/usb/class/usb-midi.c 1.22 vs edited =====
--- 1.22/drivers/usb/class/usb-midi.c	Tue Sep  2 11:40:27 2003
+++ edited/drivers/usb/class/usb-midi.c	Tue Sep 23 11:36:03 2003
@@ -1750,7 +1750,7 @@
 	return 0;
 
  error_end:
-	if ( mdevs != NULL && devices > 0 ) {
+	if ( mdevs != NULL ) {
 		for ( i=0 ; i<devices ; i++ ) {
 			if ( mdevs[i] != NULL ) {
 				unregister_sound_midi( mdevs[i]->dev_midi );

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/fs/afs/cell.c]
> START -->
>   58:	cell = kmalloc(sizeof(afs_cell_t) + strlen(name) + 1,GFP_KERNEL);
>   59:	if (!cell) {
<snip>
>  126: error:
>  127:	up_write(&afs_cells_sem);
>  128:	kfree(afs_cell_root);
> END -->

Yes, this looks like a bug/typo.  Patch below.  David, this look ok?

thanks,
-chris

===== fs/afs/cell.c 1.2 vs edited =====
--- 1.2/fs/afs/cell.c	Tue Sep  9 03:21:38 2003
+++ edited/fs/afs/cell.c	Tue Sep 23 11:57:26 2003
@@ -145,7 +145,7 @@
 	printk("kAFS: bad VL server IP address: '%s'\n",vllist);
  error:
 	up_write(&afs_cells_sem);
-	kfree(afs_cell_root);
+	kfree(cell);
 	return ret;
 } /* end afs_cell_create() */
 

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/net/irda/irlmp.c]
> [FUNC:  irlmp_open_lsap]
> [LINES: 161-183]
> [VAR:   self]
> START -->
>  161:	self = kmalloc(sizeof(struct lsap_cb), GFP_ATOMIC);
>  162:	if (self == NULL) {
>  163:		ERROR("%s: can't allocate memory", __FUNCTION__);
>  164:		return NULL;
> END -->
>  183:	ASSERT(notify->instance != NULL, return NULL;);

Yes, this is a bug.  Patch below.  Jean, look ok?

thanks,
-chris

===== net/irda/irlmp.c 1.29 vs edited =====
--- 1.29/net/irda/irlmp.c	Sat Sep 20 00:48:35 2003
+++ edited/net/irda/irlmp.c	Tue Sep 23 12:09:02 2003
@@ -178,7 +178,7 @@
 
 	init_timer(&self->watchdog_timer);
 
-	ASSERT(notify->instance != NULL, return NULL;);
+	ASSERT(notify->instance != NULL, goto error;);
 	self->notify = *notify;
 
 	self->lsap_state = LSAP_DISCONNECTED;
@@ -188,6 +188,9 @@
 		       (long) self, NULL);
 
 	return self;
+error:
+	kfree(self);
+	return NULL;
 }
 
 /*

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/net/sunrpc/auth_gss/gss_mech_switch.c]
> START -->
>   67:	if (!(gm = kmalloc(sizeof(*gm), GFP_KERNEL))) {
>   68:		printk("Failed to allocate memory in gss_mech_register");
>   69:		return -1;
>   70:	}
>   71:	gm->gm_oid.len = mech_type->len;
>   72:	if (!(gm->gm_oid.data = kmalloc(mech_type->len, GFP_KERNEL))) {
>   73:		printk("Failed to allocate memory in gss_mech_register");
> END -->
>   74:		return -1;
<snip>
> [FILE:  2.6.0-test5/net/sunrpc/auth_gss/gss_pseudoflavors.c]
> [FUNC:  gss_register_triple]
> START -->
>   74:	if (!(triple = kmalloc(sizeof(*triple), GFP_KERNEL))) {
>   75:		printk("Alloc failed in gss_register_triple");
> GOTO -->
>   86:		goto err_unlock;
> END -->
>   97:	return -1;

Yes, these are both bugs.  Patch below.  Trond, this look ok?

thanks,
-chris

===== net/sunrpc/auth_gss/gss_mech_switch.c 1.2 vs edited =====
--- 1.2/net/sunrpc/auth_gss/gss_mech_switch.c	Wed Jun 11 19:25:40 2003
+++ edited/net/sunrpc/auth_gss/gss_mech_switch.c	Tue Sep 23 12:18:26 2003
@@ -71,6 +71,7 @@
 	gm->gm_oid.len = mech_type->len;
 	if (!(gm->gm_oid.data = kmalloc(mech_type->len, GFP_KERNEL))) {
 		printk("Failed to allocate memory in gss_mech_register");
+		kfree(gm);
 		return -1;
 	}
 	memcpy(gm->gm_oid.data, mech_type->data, mech_type->len);
===== net/sunrpc/auth_gss/gss_pseudoflavors.c 1.1 vs edited =====
--- 1.1/net/sunrpc/auth_gss/gss_pseudoflavors.c	Sun Jan 12 13:40:13 2003
+++ edited/net/sunrpc/auth_gss/gss_pseudoflavors.c	Tue Sep 23 12:24:47 2003
@@ -93,6 +93,7 @@
 
 err_unlock:
 	spin_unlock(&registered_triples_lock);
+	kfree(triple);
 err:
 	return -1;
 }

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* David Yu Chen (dychen@stanford.edu) wrote:
> [FILE:  2.6.0-test5/sound/oss/ymfpci.c]
> START -->
> 2530:	if ((codec = kmalloc(sizeof(ymfpci_t), GFP_KERNEL)) == NULL) {
> 2531:		printk(KERN_ERR "ymfpci: no core\n");
> 2532:		return -ENOMEM;
> 2533:	}
> 2534:	memset(codec, 0, sizeof(*codec));
> 2535:
>         ... DELETED 11 lines ...
> 2547:		goto out_free;
> 2548:	}
> 2549:
> 2550:	if ((codec->reg_area_virt = ioremap(base, 0x8000)) == NULL) {
> 2551:		printk(KERN_ERR "ymfpci: unable to map registers\n");
> GOTO -->
> 2552:		goto out_release_region;
> 2553:	}
> 2554:
> 2555:	pci_set_master(pcidev);
> 2556:
> 2557:	printk(KERN_INFO "ymfpci: %s at 0x%lx IRQ %d\n",
>         ... DELETED 78 lines ...
> 2636: out_release_region:
> 2637:	release_mem_region(pci_resource_start(pcidev, 0), 0x8000);
> 2638: out_free:
> 2639:	if (codec->ac97_codec[0])
> 2640:		ac97_release_codec(codec->ac97_codec[0]);
> END -->
> 2641:	return -ENODEV;

Yes, this looks like a bug.  Patch below.  Alan, this look ok?

thanks,
-chris

===== sound/oss/ymfpci.c 1.38 vs edited =====
--- 1.38/sound/oss/ymfpci.c	Tue Aug 26 09:25:41 2003
+++ edited/sound/oss/ymfpci.c	Tue Sep 23 12:42:45 2003
@@ -2638,6 +2638,7 @@
  out_free:
 	if (codec->ac97_codec[0])
 		ac97_release_codec(codec->ac97_codec[0]);
+	kfree(codec);
 	return -ENODEV;
 }
 

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Jean Tourrilhes]

On Tue, Sep 23, 2003 at 01:14:30PM -0700, Chris Wright wrote:
> * David Yu Chen (dychen@stanford.edu) wrote:
> > [FILE:  2.6.0-test5/net/irda/irlmp.c]
> > [FUNC:  irlmp_open_lsap]
> > [LINES: 161-183]
> > [VAR:   self]
> > START -->
> >  161:	self = kmalloc(sizeof(struct lsap_cb), GFP_ATOMIC);
> >  162:	if (self == NULL) {
> >  163:		ERROR("%s: can't allocate memory", __FUNCTION__);
> >  164:		return NULL;
> > END -->
> >  183:	ASSERT(notify->instance != NULL, return NULL;);
> 
> Yes, this is a bug.  Patch below.  Jean, look ok?
> 
> thanks,
> -chris

	Yep, looks like the right thing. But, I would prefer a slighly
different fix. Would you mind moving the assert at the top of the
function ?
	Just like this :
--------------------------------------------------------------------
--- linux/net/irda/irlmp.d1.c   Tue Sep 23 13:19:37 2003
+++ linux/net/irda/irlmp.c      Tue Sep 23 13:19:56 2003
@@ -146,6 +146,7 @@ struct lsap_cb *irlmp_open_lsap(__u8 sls
        ASSERT(notify != NULL, return NULL;);
        ASSERT(irlmp != NULL, return NULL;);
        ASSERT(irlmp->magic == LMP_MAGIC, return NULL;);
+       ASSERT(notify->instance != NULL, return NULL;);
 
        /*  Does the client care which Source LSAP selector it gets?  */
        if (slsap_sel == LSAP_ANY) {
@@ -178,7 +179,6 @@ struct lsap_cb *irlmp_open_lsap(__u8 sls
 
        init_timer(&self->watchdog_timer);
 
-       ASSERT(notify->instance != NULL, return NULL;);
        self->notify = *notify;
 
        self->lsap_state = LSAP_DISCONNECTED;
--------------------------------------------------------------------

	This is simpler and more efficient ;-)

	Thanks...

	Jean

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* Jean Tourrilhes (jt@bougret.hpl.hp.com) wrote:
> 
> 	This is simpler and more efficient ;-)

Yes, I agree, your patch is better.

-thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Greg KH]

On Tue, Sep 23, 2003 at 01:13:50PM -0700, Chris Wright wrote:
> * David Yu Chen (dychen@stanford.edu) wrote:
> > Leaks if devices == 0 ?  Error_end only frees mdevs if (devices > 0), 
> > but for mdevs=kmalloc(0), the slab allocator may still actually return memory
> > [FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
> > [FUNC:  alloc_usb_midi_device]
> > [LINES: 1621-1772]
> > [VAR:   mdevs]
> > 1616:	devices = inDevs > outDevs ? inDevs : outDevs;
> > 1617:	devices = maxdevices > devices ? devices : maxdevices;
> > 1618:
> > 1619:	/* obtain space for device name (iProduct) if not known. */
> > 1620:	if ( ! u->deviceName ) {
> > START -->
> > 1621:		mdevs = (struct usb_mididev **)
> > 1622:			kmalloc(sizeof(struct usb_mididevs *)*devices
> > 1623:				+ sizeof(char) * 256, GFP_KERNEL);
> <snip>
> > GOTO -->
> > 1715:			goto error_end;
> <snip>
> > END -->
> > 1772:	return -ENOMEM;
> > [FILE:  2.6.0-test5/drivers/usb/class/usb-midi.c]
> > START -->
> > 1625:		mdevs = (struct usb_mididev **)
> > 1626:			kmalloc(sizeof(struct usb_mididevs *)*devices, GFP_KERNEL);
> <snip>
> > GOTO -->
> > 1715:			goto error_end;
> <snip>
> > END -->
> > 1772:	return -ENOMEM;
> 
> Yes, these are bugs.  Patch below.  Greg, this look ok?

Don't know, Vojtech said he would fix these up already.  Try asking him
:)

thanks,

greg k-h

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> 
> Don't know, Vojtech said he would fix these up already.  Try asking him
> :)

Ah, I missed that.  Will do.
thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Chris Wright]

* Greg KH (greg@kroah.com) wrote:
> Don't know, Vojtech said he would fix these up already.  Try asking him
> :)

I checked with Vojtech, he said the patch looked OK.  Can you apply?

thanks,
-chris

===== drivers/usb/class/usb-midi.c 1.22 vs edited =====
--- 1.22/drivers/usb/class/usb-midi.c	Tue Sep  2 11:40:27 2003
+++ edited/drivers/usb/class/usb-midi.c	Tue Sep 23 11:36:03 2003
@@ -1750,7 +1750,7 @@
 	return 0;
 
  error_end:
-	if ( mdevs != NULL && devices > 0 ) {
+	if ( mdevs != NULL ) {
 		for ( i=0 ; i<devices ; i++ ) {
 			if ( mdevs[i] != NULL ) {
 				unregister_sound_midi( mdevs[i]->dev_midi );

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Greg KH]

On Tue, Sep 23, 2003 at 03:14:56PM -0700, Chris Wright wrote:
> * Greg KH (greg@kroah.com) wrote:
> > Don't know, Vojtech said he would fix these up already.  Try asking him
> > :)
> 
> I checked with Vojtech, he said the patch looked OK.  Can you apply?

Applied, thanks.

greg k-h

Re: [CHECKER] 32 Memory Leaks on Error Paths

[David Howells]

> * David Yu Chen (dychen@stanford.edu) wrote:
> > [FILE:  2.6.0-test5/fs/afs/cell.c]
> > START -->
> >   58:	cell = kmalloc(sizeof(afs_cell_t) + strlen(name) + 1,GFP_KERNEL);
> >   59:	if (!cell) {
> <snip>
> >  126: error:
> >  127:	up_write(&afs_cells_sem);
> >  128:	kfree(afs_cell_root);
> > END -->
> 
> Yes, this looks like a bug/typo.  Patch below.  David, this look ok?

Yes. I've applied it to my local development version.

Thanks,
David

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Pete Zaitcev]

On Mon, Sep 29, 2003 at 02:48:33PM -0300, Marcelo Tosatti wrote:
> > > Date: Tue, 23 Sep 2003 13:15:15 -0700
> > > From: Chris Wright <chrisw@osdl.org>
> > 
> > > --- 1.38/sound/oss/ymfpci.c	Tue Aug 26 09:25:41 2003
> > > +++ edited/sound/oss/ymfpci.c	Tue Sep 23 12:42:45 2003
> > > @@ -2638,6 +2638,7 @@
> > >   out_free:
> > >  	if (codec->ac97_codec[0])
> > >  		ac97_release_codec(codec->ac97_codec[0]);
> > > +	kfree(codec);
> > >  	return -ENODEV;
> > >  }

I got around to fixing both missing kfrees and the schedule
under lock (for 2.4). Alan should be happy now.

-- Pete

P.S. I do not want to fix the 2.6. For crying out loud, the driver
was copied bit by bit from Jaroslav's ALSA code. Now that we
have ALSA in the kernel, can we let go already?  I do not see
why anyone would want to use my driver instead of Jaroslav's driver.

diff -ur -X dontdiff linux-2.4.23-pre9/drivers/sound/ymfpci.c linux-2.4.23-pre9-nip/drivers/sound/ymfpci.c
--- linux-2.4.23-pre9/drivers/sound/ymfpci.c	2003-11-05 11:44:34.000000000 -0800
+++ linux-2.4.23-pre9-nip/drivers/sound/ymfpci.c	2003-11-05 16:32:40.000000000 -0800
@@ -152,22 +152,19 @@
 	writel(val, codec->reg_area_virt + offset);
 }
 
-static int ymfpci_codec_ready(ymfpci_t *codec, int secondary, int sched)
+static int ymfpci_codec_ready(ymfpci_t *unit, int secondary)
 {
-	signed long end_time;
+	enum { READY_STEP = 10 };
 	u32 reg = secondary ? YDSXGR_SECSTATUSADR : YDSXGR_PRISTATUSADR;
+	int i;
 
-	end_time = jiffies + 3 * (HZ / 4);
-	do {
-		if ((ymfpci_readw(codec, reg) & 0x8000) == 0)
+	for (i = 0; i < ((3*1000)/4) / READY_STEP; i++) {
+		if ((ymfpci_readw(unit, reg) & 0x8000) == 0)
 			return 0;
-		if (sched) {
-			set_current_state(TASK_UNINTERRUPTIBLE);
-			schedule_timeout(1);
-		}
-	} while (end_time - (signed long)jiffies >= 0);
+		mdelay(READY_STEP);
+	}
 	printk(KERN_ERR "ymfpci_codec_ready: codec %i is not ready [0x%x]\n",
-	    secondary, ymfpci_readw(codec, reg));
+	    secondary, ymfpci_readw(unit, reg));
 	return -EBUSY;
 }
 
@@ -178,38 +175,35 @@
 
 	spin_lock(&codec->ac97_lock);
 	/* XXX Do make use of dev->id */
-	ymfpci_codec_ready(codec, 0, 0);
+	ymfpci_codec_ready(codec, 0);
 	cmd = ((YDSXG_AC97WRITECMD | reg) << 16) | val;
 	ymfpci_writel(codec, YDSXGR_AC97CMDDATA, cmd);
 	spin_unlock(&codec->ac97_lock);
 }
 
-static u16 _ymfpci_codec_read(ymfpci_t *unit, u8 reg)
+static u16 ymfpci_codec_read(struct ac97_codec *dev, u8 reg)
 {
+	ymfpci_t *unit = dev->private_data;
+	u16 ret;
 	int i;
 
-	if (ymfpci_codec_ready(unit, 0, 0))
-		return ~0;
+	spin_lock(&unit->ac97_lock);
+	if (ymfpci_codec_ready(unit, 0))
+		goto out_err;
 	ymfpci_writew(unit, YDSXGR_AC97CMDADR, YDSXG_AC97READCMD | reg);
-	if (ymfpci_codec_ready(unit, 0, 0))
-		return ~0;
+	if (ymfpci_codec_ready(unit, 0))
+		goto out_err;
 	if (unit->pci->device == PCI_DEVICE_ID_YAMAHA_744 && unit->rev < 2) {
 		for (i = 0; i < 600; i++)
 			ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
 	}
-	return ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
-}
-
-static u16 ymfpci_codec_read(struct ac97_codec *dev, u8 reg)
-{
-	ymfpci_t *unit = dev->private_data;
-	u16 ret;
-	
-	spin_lock(&unit->ac97_lock);
-	ret = _ymfpci_codec_read(unit, reg);
+	ret = ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
 	spin_unlock(&unit->ac97_lock);
-	
 	return ret;
+
+ out_err:
+	spin_unlock(&unit->ac97_lock);
+	return ~0;
 }
 
 /*
@@ -2123,7 +2117,7 @@
 	int i;
 
 	ymfpci_aclink_reset(unit->pci);
-	ymfpci_codec_ready(unit, 0, 1);		/* prints diag if not ready. */
+	ymfpci_codec_ready(unit, 0);		/* prints diag if not ready. */
 
 #ifdef CONFIG_SOUND_YMFPCI_LEGACY
 	/* XXX At this time the legacy registers are probably deprogrammed. */
@@ -2549,7 +2543,7 @@
 	    (char *)ent->driver_data, base, pcidev->irq);
 
 	ymfpci_aclink_reset(pcidev);
-	if (ymfpci_codec_ready(codec, 0, 1) < 0)
+	if (ymfpci_codec_ready(codec, 0) < 0)
 		goto out_unmap;
 
 #ifdef CONFIG_SOUND_YMFPCI_LEGACY
@@ -2625,6 +2619,7 @@
  out_release_region:
 	release_mem_region(pci_resource_start(pcidev, 0), 0x8000);
  out_free:
+	kfree(codec);
 	return -ENODEV;
 }
 
@@ -2652,6 +2647,7 @@
 		unload_uart401(&codec->mpu_data);
 	}
 #endif /* CONFIG_SOUND_YMFPCI_LEGACY */
+	kfree(codec);
 }
 
 MODULE_AUTHOR("Jaroslav Kysela");

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Pete Zaitcev]

> Date: Mon, 29 Sep 2003 14:48:33 -0300 (BRT)
> From: Marcelo Tosatti <marcelo.tosatti@cyclades.com>

> > > Date: Tue, 23 Sep 2003 13:15:15 -0700
> > > From: Chris Wright <chrisw@osdl.org>
> > 
> > > --- 1.38/sound/oss/ymfpci.c	Tue Aug 26 09:25:41 2003
> > > +++ edited/sound/oss/ymfpci.c	Tue Sep 23 12:42:45 2003
> > > @@ -2638,6 +2638,7 @@
> > >   out_free:
> > >  	if (codec->ac97_codec[0])
> > >  		ac97_release_codec(codec->ac97_codec[0]);
> > > +	kfree(codec);
> > >  	return -ENODEV;
> > 
> > Someone was savaging sound code, adding these bugs.
> > For 2.6, the above patch is probably ok; it's immaterial,
> > because ALSA won long ago.
> > 
> > A patch for 2.4 to undo the damage is attached.
> 
> So let me see if I get it right: The above "+ kfree(codec)" addition caused the bug?

No, bugs were added for 2.4.22 (a diff between 2.4.21 & .22
removes kfree's). Chris adds some kfrees back, but not all.

Also, 2.4.22 adds spinlocks around schedule(). My bigger patch
replaces them with semaphores, because interrupts are never
involved and they were done from a process context only.
However, Alan noted that ac97 operations can be called with
interrupts closed, and so the right approach is to retain
spinlocks, but replace schedule() with mdelay(). This is gonna
be one huge sleep under cli... Nonetheless, I'll send a better
patch soonish.

-- Pete

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Marcelo Tosatti]

On Wed, 24 Sep 2003, Pete Zaitcev wrote:

> > Date: Tue, 23 Sep 2003 13:15:15 -0700
> > From: Chris Wright <chrisw@osdl.org>
> 
> > --- 1.38/sound/oss/ymfpci.c	Tue Aug 26 09:25:41 2003
> > +++ edited/sound/oss/ymfpci.c	Tue Sep 23 12:42:45 2003
> > @@ -2638,6 +2638,7 @@
> >   out_free:
> >  	if (codec->ac97_codec[0])
> >  		ac97_release_codec(codec->ac97_codec[0]);
> > +	kfree(codec);
> >  	return -ENODEV;
> >  }
> 
> Someone was savaging sound code, adding these bugs.
> For 2.6, the above patch is probably ok; it's immaterial,
> because ALSA won long ago.
> 
> A patch for 2.4 to undo the damage is attached.

Pete, 

So let me see if I get it right: The above "+ kfree(codec)" addition caused the bug?

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Pete Zaitcev]

> > -	spin_lock(&codec->ac97_lock);
> > +	down(&unit->ac97_lock);
> >  	/* XXX Do make use of dev->id */
> > -	ymfpci_codec_ready(codec, 0, 0);
> 
> This breaks ac97 locking and should not be applied. The core ac97
> code is called some times with interrupts disabled. That is unavoidable.
> 
> The only change that is relevant is the kfree

In that case, whoever added spinlocks should have removed
schedule() from ymfpci_ready_wait().

-- Pete

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Alan Cox]

On Mer, 2003-09-24 at 05:13, Pete Zaitcev wrote: 
> -	spin_lock(&codec->ac97_lock);
> +	down(&unit->ac97_lock);
>  	/* XXX Do make use of dev->id */
> -	ymfpci_codec_ready(codec, 0, 0);

This breaks ac97 locking and should not be applied. The core ac97
code is called some times with interrupts disabled. That is unavoidable.

The only change that is relevant is the kfree

Re: [CHECKER] 32 Memory Leaks on Error Paths

[Pete Zaitcev]

> Date: Tue, 23 Sep 2003 13:15:15 -0700
> From: Chris Wright <chrisw@osdl.org>

> --- 1.38/sound/oss/ymfpci.c	Tue Aug 26 09:25:41 2003
> +++ edited/sound/oss/ymfpci.c	Tue Sep 23 12:42:45 2003
> @@ -2638,6 +2638,7 @@
>   out_free:
>  	if (codec->ac97_codec[0])
>  		ac97_release_codec(codec->ac97_codec[0]);
> +	kfree(codec);
>  	return -ENODEV;
>  }

Someone was savaging sound code, adding these bugs.
For 2.6, the above patch is probably ok; it's immaterial,
because ALSA won long ago.

A patch for 2.4 to undo the damage is attached.

-- Pete

diff -ur -X dontdiff linux-2.4.23-pre5/drivers/sound/ymfpci.c linux-2.4.23-pre5-nip/drivers/sound/ymfpci.c
--- linux-2.4.23-pre5/drivers/sound/ymfpci.c	2003-09-23 17:41:04.000000000 -0700
+++ linux-2.4.23-pre5-nip/drivers/sound/ymfpci.c	2003-09-23 20:44:46.000000000 -0700
@@ -173,43 +173,40 @@
 
 static void ymfpci_codec_write(struct ac97_codec *dev, u8 reg, u16 val)
 {
-	ymfpci_t *codec = dev->private_data;
+	ymfpci_t *unit = dev->private_data;
 	u32 cmd;
 
-	spin_lock(&codec->ac97_lock);
+	down(&unit->ac97_lock);
 	/* XXX Do make use of dev->id */
-	ymfpci_codec_ready(codec, 0, 0);
+	ymfpci_codec_ready(unit, 0, 0);
 	cmd = ((YDSXG_AC97WRITECMD | reg) << 16) | val;
-	ymfpci_writel(codec, YDSXGR_AC97CMDDATA, cmd);
-	spin_unlock(&codec->ac97_lock);
+	ymfpci_writel(unit, YDSXGR_AC97CMDDATA, cmd);
+	up(&unit->ac97_lock);
 }
 
-static u16 _ymfpci_codec_read(ymfpci_t *unit, u8 reg)
+static u16 ymfpci_codec_read(struct ac97_codec *dev, u8 reg)
 {
+	ymfpci_t *unit = dev->private_data;
 	int i;
+	u16 ret;
 
+	down(&unit->ac97_lock);
 	if (ymfpci_codec_ready(unit, 0, 0))
-		return ~0;
+		goto out_none;
 	ymfpci_writew(unit, YDSXGR_AC97CMDADR, YDSXG_AC97READCMD | reg);
 	if (ymfpci_codec_ready(unit, 0, 0))
-		return ~0;
+		goto out_none;
 	if (unit->pci->device == PCI_DEVICE_ID_YAMAHA_744 && unit->rev < 2) {
 		for (i = 0; i < 600; i++)
 			ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
 	}
-	return ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
-}
-
-static u16 ymfpci_codec_read(struct ac97_codec *dev, u8 reg)
-{
-	ymfpci_t *unit = dev->private_data;
-	u16 ret;
-	
-	spin_lock(&unit->ac97_lock);
-	ret = _ymfpci_codec_read(unit, reg);
-	spin_unlock(&unit->ac97_lock);
-	
+	ret = ymfpci_readw(unit, YDSXGR_PRISTATUSDATA);
+	up(&unit->ac97_lock);
 	return ret;
+
+out_none:
+	up(&unit->ac97_lock);
+	return ~0;
 }
 
 /*
@@ -2526,7 +2523,7 @@
 
 	spin_lock_init(&codec->reg_lock);
 	spin_lock_init(&codec->voice_lock);
-	spin_lock_init(&codec->ac97_lock);
+	init_MUTEX(&codec->ac97_lock);
 	init_MUTEX(&codec->open_sem);
 	INIT_LIST_HEAD(&codec->states);
 	codec->pci = pcidev;
@@ -2625,6 +2622,7 @@
  out_release_region:
 	release_mem_region(pci_resource_start(pcidev, 0), 0x8000);
  out_free:
+	kfree(codec);
 	return -ENODEV;
 }
 
@@ -2652,6 +2650,7 @@
 		unload_uart401(&codec->mpu_data);
 	}
 #endif /* CONFIG_SOUND_YMFPCI_LEGACY */
+	kfree(codec);
 }
 
 MODULE_AUTHOR("Jaroslav Kysela");
diff -ur -X dontdiff linux-2.4.23-pre5/drivers/sound/ymfpci.h linux-2.4.23-pre5-nip/drivers/sound/ymfpci.h
--- linux-2.4.23-pre5/drivers/sound/ymfpci.h	2003-09-23 17:56:01.000000000 -0700
+++ linux-2.4.23-pre5-nip/drivers/sound/ymfpci.h	2003-09-23 20:46:06.000000000 -0700
@@ -275,7 +275,7 @@
 
 	spinlock_t reg_lock;
 	spinlock_t voice_lock;
-	spinlock_t ac97_lock;
+	struct semaphore ac97_lock;
 
 	/* soundcore stuff */
 	int dev_audio;