2001-03-16 02:35:46

by Dawson Engler

[permalink] [raw]
Subject: [CHECKER] big stack variables

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


2001-03-16 02:57:06

by Jeff Dike

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

[email protected] 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


2001-03-16 03:08:16

by Andrew Morton

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

Jeff Dike wrote:
>
> [email protected] 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.

-

2001-03-16 07:21:15

by Dawson Engler

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

> [email protected] 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

2001-03-16 08:23:17

by Andrew Morton

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

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.

2001-03-16 16:46:55

by Jeff Dike

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

[email protected] 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();


2001-03-16 16:59:35

by Alexander Viro

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables



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()...

2001-03-16 18:26:39

by Jeff Dike

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

[email protected] 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


2001-03-17 04:50:54

by Jeff Dike

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

[email protected] 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


2001-03-17 11:40:50

by David Weinehall

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

On Sat, Mar 17, 2001 at 01:01:22AM -0500, Jeff Dike wrote:
> [email protected] 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 <[email protected]> /> Northern lights wander \\
// Project MCA Linux hacker // Dance across the winter sky //
\> http://www.acc.umu.se/~tao/ </ Full colour fire </

2001-03-18 16:31:20

by Jeff Dike

[permalink] [raw]
Subject: Re: [CHECKER] big stack variables

[email protected] 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