[CHECKER] big stack variables

[CHECKER] big stack variables

[Dawson Engler]

Hi,

enclosed are 22 functions in 2.4.1 that appear to allocate stack
variables >= 1024 bytes.  As usual, please report any false positives
so we can fix our checkers.

Dawson
-----------------------------------------------------------------------
/u2/engler/mc/2.4.1/drivers/isdn/sc/message.c:52:dump_messages: ERROR:VAR: suspicious sized variable: 'dpm' = 4112 bytes
/u2/engler/mc/2.4.1/drivers/sound/emu10k1/audio.c:906:emu10k1_audio_ioctl: ERROR:VAR: suspicious sized variable: 'buf' = 4016 bytes
/u2/engler/mc/2.4.1/drivers/i2o/i2o_proc.c:955:i2o_proc_read_drivers_stored: ERROR:VAR: suspicious sized variable: 'result' = 3596 bytes
/u2/engler/mc/2.4.1/drivers/scsi/qlogicfc.c:861:isp2x00_make_portdb: ERROR:VAR: suspicious sized variable: 'temp' = 3072 bytes
/u2/engler/mc/2.4.1/drivers/i2o/i2o_proc.c:840:i2o_proc_read_ddm_table: ERROR:VAR: suspicious sized variable: 'result' = 2828 bytes
/u2/engler/mc/2.4.1/drivers/cdrom/optcd.c:1625:cdromread: ERROR:VAR: suspicious sized variable: 'buf' = 2646 bytes
/u2/engler/mc/2.4.1/drivers/i2o/i2o_proc.c:2261:i2o_proc_read_lan_mcast_addr: ERROR:VAR: suspicious sized variable: 'result' = 2060 bytes
/u2/engler/mc/2.4.1/drivers/i2o/i2o_proc.c:2492:i2o_proc_read_lan_alt_addr: ERROR:VAR: suspicious sized variable: 'result' = 2060 bytes
/u2/engler/mc/2.4.1/drivers/i2o/i2o_proc.c:1044:i2o_proc_read_groups: ERROR:VAR: suspicious sized variable: 'result' = 2060 bytes
/u2/engler/mc/2.4.1/fs/ntfs/super.c:335:ntfs_get_free_cluster_count: ERROR:VAR: suspicious sized variable: 'bits' = 2048 bytes
/u2/engler/mc/2.4.1/drivers/atm/iphase.c:2760:ia_ioctl: ERROR:VAR: suspicious sized variable: 'regs_local' = 2048 bytes
/u2/engler/mc/2.4.1/drivers/block/cpqarray.c:1156:ida_ioctl: ERROR:VAR: suspicious sized variable: 'my_io' = 1296 bytes
/u2/engler/mc/2.4.1/net/wanrouter/wanmain.c:578:device_new_if: ERROR:VAR: suspicious sized variable: 'conf' = 1220 bytes
/u2/engler/mc/2.4.1/drivers/net/zlib.c:4216:huft_build: ERROR:VAR: suspicious sized variable: 'v' = 1152 bytes
/u2/engler/mc/2.4.1/drivers/net/zlib.c:4501:inflate_trees_fixed: ERROR:VAR: suspicious sized variable: 'c' = 1152 bytes
/u2/engler/mc/2.4.1/drivers/cdrom/cdrom.c:734:cdrom_slot_status: ERROR:VAR: suspicious sized variable: 'info' = 1032 bytes
/u2/engler/mc/2.4.1/drivers/cdrom/cdrom.c:800:cdrom_select_disc: ERROR:VAR: suspicious sized variable: 'info' = 1032 bytes
/u2/engler/mc/2.4.1/drivers/cdrom/cdrom.c:758:cdrom_number_of_slots: ERROR:VAR: suspicious sized variable: 'info' = 1032 bytes
/u2/engler/mc/2.4.1/drivers/cdrom/cdrom.c:1538:cdrom_ioctl: ERROR:VAR: suspicious sized variable: 'info' = 1032 bytes
/u2/engler/mc/2.4.1/fs/nfs/nfsroot.c:238:root_nfs_name: ERROR:VAR: suspicious sized variable: 'buf' = 1024 bytes
/u2/engler/mc/2.4.1/net/bridge/br_ioctl.c:86:br_ioctl_device: ERROR:VAR: suspicious sized variable: 'indices' = 1024 bytes
/u2/engler/mc/2.4.1/drivers/isdn/pcbit/drv.c:444:pcbit_writecmd: ERROR:VAR: suspicious sized variable: 'cbuf' = 1024 bytes

Re: [CHECKER] big stack variables

[Jeff Dike]

engler@csl.Stanford.EDU said:
> As usual, please report any false positives so we can fix our
> checkers.

Not a false positive, but a false negative:

the tty_struct locals at lines 1994 and 2029 in tty_register_devfs and 
tty_unregister_devfs, respectively, in the 2.4.2 drivers/char/tty_io.c.

Nice work, BTW.

				Jeff

Re: [CHECKER] big stack variables

[Andrew Morton]

Jeff Dike wrote:
> 
> engler@csl.Stanford.EDU said:
> > As usual, please report any false positives so we can fix our
> > checkers.
> 
> Not a false positive, but a false negative:
> 
> the tty_struct locals at lines 1994 and 2029 in tty_register_devfs and
> tty_unregister_devfs, respectively, in the 2.4.2 drivers/char/tty_io.c.

and sanitize_e820_map()

> Nice work, BTW.

Yep.

-

Re: [CHECKER] big stack variables

[Dawson Engler]

> engler@csl.Stanford.EDU said:
> > As usual, please report any false positives so we can fix our
> > checkers.
> 
> Not a false positive, but a false negative:
> 
> the tty_struct locals at lines 1994 and 2029 in tty_register_devfs and 
> tty_unregister_devfs, respectively, in the 2.4.2 drivers/char/tty_io.c.

Turns out we didn't have CONFIG_DEVFS_FS defined.  Big time fun when it is:

/u2/engler/mc/2.4.1/drivers/char/tty_io.c:1996:tty_register_devfs: ERROR:VAR:1996:1996: suspicious var 'tty' = 3112 bytes
/u2/engler/mc/2.4.1/drivers/char/tty_io.c:2007:tty_register_devfs: ERROR:FN:2007:2007: function stack consumes = 3146 bytes
/u2/engler/mc/2.4.1/drivers/char/tty_io.c:2031:tty_unregister_devfs: ERROR:VAR:2031:2031: suspicious var 'tty' = 3112 bytes
/u2/engler/mc/2.4.1/drivers/char/tty_io.c:2042:tty_unregister_devfs: ERROR:FN:2042:2042: function stack consumes = 3148 bytes

Right now we've just gone in and put =y for all options in .config --- is
there a more principled approach that will get more coverage?

> Nice work, BTW.

Thanks!  If you have any other ideas of things to check for, do let us
know.  We're mainly just going after things we've found in comments and
code.  We have about another 600 potential bugs to report, but are
going over them to try to make sure they are reasonable.

Dawson

Re: [CHECKER] big stack variables

[Andrew Morton]

Dawson Engler wrote:
> 
> Turns out we didn't have CONFIG_DEVFS_FS defined.  Big time fun when it is:
> 
> /u2/engler/mc/2.4.1/drivers/char/tty_io.c:1996:tty_register_devfs: ERROR:VAR:1996:1996: suspicious var 'tty' = 3112 bytes

I've got my nose stuck in tty_io.c at present - I'll fix this this one.

Re: [CHECKER] big stack variables

[Alexander Viro]

On Fri, 16 Mar 2001, Jeff Dike wrote:

> +#endif
> +#ifdef CONFIG_STDIO_CONSOLE
> +	stdio_console_init();
>  #endif

Erm... That piece is UML-only.

ObUML: something fishy happens in UML with multiple exec() in PID 1. Try to
say "telinit u" (or just boot with init=/bin/sh and say exec /sbin/init)
and you've got a nice panic()...

Re: [CHECKER] big stack variables

[Jeff Dike]

andrewm@uow.edu.au said:
> I've got my nose stuck in tty_io.c at present - I'll fix this this
> one. 

This is the patch I've been carrying around in the UML pool since this bit me:

diff -Naur -X exclude-files orig/drivers/char/tty_io.c um/drivers/char/tty_io.c
--- orig/drivers/char/tty_io.c	Thu Feb 22 11:53:50 2001
+++ um/drivers/char/tty_io.c	Thu Feb 22 11:54:55 2001
@@ -1991,12 +1991,11 @@
 {
 #ifdef CONFIG_DEVFS_FS
 	umode_t mode = S_IFCHR | S_IRUSR | S_IWUSR;
-	struct tty_struct tty;
+	kdev_t device = MKDEV (driver->major, minor);
+	int idx = minor - driver->minor_start;
 	char buf[32];
 
-	tty.driver = *driver;
-	tty.device = MKDEV (driver->major, minor);
-	switch (tty.device) {
+	switch (device) {
 		case TTY_DEV:
 		case PTMX_DEV:
 			mode |= S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
@@ -2017,23 +2016,21 @@
 	     (driver->major < UNIX98_PTY_SLAVE_MAJOR + UNIX98_NR_MAJORS) )
 		flags |= DEVFS_FL_CURRENT_OWNER;
 #  endif
-	devfs_register (NULL, tty_name (&tty, buf), flags | DEVFS_FL_DEFAULT,
+	sprintf(buf, driver->name, idx + driver->name_base);
+	devfs_register (NULL, buf, flags | DEVFS_FL_DEFAULT,
 			driver->major, minor, mode, &tty_fops, NULL);
 #endif /* CONFIG_DEVFS_FS */
 }
 
-void tty_unregister_devfs (struct tty_driver *driver, unsigned minor)
+void tty_unregister_devfs (struct tty_driver *driver, unsigned int minor)
 {
 #ifdef CONFIG_DEVFS_FS
 	void * handle;
-	struct tty_struct tty;
+	int idx = minor - driver->minor_start;
 	char buf[32];
 
-	tty.driver = *driver;
-	tty.device = MKDEV(driver->major, minor);
-	
-	handle = devfs_find_handle (NULL, tty_name (&tty, buf),
-				    driver->major, minor,
+	sprintf(buf, driver->name, idx + driver->name_base);
+	handle = devfs_find_handle (NULL, buf, driver->major, minor,
 				    DEVFS_SPECIAL_CHR, 0);
 	devfs_unregister (handle);
 #endif /* CONFIG_DEVFS_FS */
@@ -2192,6 +2189,9 @@
 #endif
 #ifdef CONFIG_HWC
         hwc_console_init();
+#endif
+#ifdef CONFIG_STDIO_CONSOLE
+	stdio_console_init();
 #endif
 #ifdef CONFIG_SERIAL_21285_CONSOLE
 	rs285_console_init();

Re: [CHECKER] big stack variables

[Jeff Dike]

viro@math.psu.edu said:
> Erm... That piece is UML-only. 

Correct, thanks for noticing that.  I was a bit over-enthusiastic with my 
cutting and pasting.  Ignore that bit.

			Jeff

Re: [CHECKER] big stack variables

[Jeff Dike]

viro@math.psu.edu said:
> ObUML: something fishy happens in UML with multiple exec() in PID 1.
> Try to say "telinit u" (or just boot with init=/bin/sh and say exec /
> sbin/init) and you've got a nice panic()... 

ObFix:  This is fixed in my current CVS.  If you're not so desperate for the 
fix, then it will be in my 2.4.3 release.  Basically, the problem was that it 
assumed that the only exec done by pid 1 was the kernel thread execing init, 
and things got exciting when that turned out not to be true.

				Jeff

Re: [CHECKER] big stack variables

[David Weinehall]

On Sat, Mar 17, 2001 at 01:01:22AM -0500, Jeff Dike wrote:
> viro@math.psu.edu said:
> > ObUML: something fishy happens in UML with multiple exec() in PID 1.
> > Try to say "telinit u" (or just boot with init=/bin/sh and say exec /
> > sbin/init) and you've got a nice panic()... 
> 
> ObFix:  This is fixed in my current CVS.  If you're not so desperate for the 
> fix, then it will be in my 2.4.3 release.  Basically, the problem was that it 
> assumed that the only exec done by pid 1 was the kernel thread execing init, 
> and things got exciting when that turned out not to be true.

ObUML (again): Any estimated time of submission to Linus?! Is this
an early v2.5-thing, or are the changes minor enough to the rest of the
tree to allow for an v2.4-merge?


/David
  _                                                                 _
 // David Weinehall <tao@acc.umu.se> /> Northern lights wander      \\
//  Project MCA Linux hacker        //  Dance across the winter sky //
\>  http://www.acc.umu.se/~tao/    </   Full colour fire           </

Re: [CHECKER] big stack variables

[Jeff Dike]

tao@acc.umu.se said:
> ObUML (again): Any estimated time of submission to Linus?! Is this an
> early v2.5-thing, or are the changes minor enough to the rest of the
> tree to allow for an v2.4-merge? 

There are almost no changes to the rest of the tree, and none of those affect 
any of the other arches, so it's an early 2.4 thing.

The reason I've waiting this long to put it in is that UML is not a normal 
port.  With the other ports, few enough people have the relevant hardware that 
if the port isn't all that great at submission time, not many people will be 
affected.  With UML, the vast majority of Linux users (the i386-using ones) 
have the relevant platform (we could get a lot closer to 100% if owners of 
non-i386 boxes would port UML to them, hint, hint :-).

I expect that getting it into the pool will be considered something of a stamp 
of approval for UML, so a lot more people will start using it.  If it sucks, 
it will affect a large number of people, word will get around that it sucks, 
and lots of people won't give it a second chance.

So, my goal for UML when it's submitted is that it make a solid, stable, UP 
virtual machine.  You can see a measure of my progress by my TODO list, which 
I post to my devel list occasionally.  The most recent one is at 
http://www.geocrawler.com/lists/3/SourceForge/709/25/5339352/

After the 2.4 submission, I'm going to start working on some more radical 
things, like SMP emulation, clustering, and artifical environments.

				Jeff