LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* a
@ 2003-02-21  8:18 Muli Ben-Yehuda
  0 siblings, 0 replies; 2+ messages in thread
From: Muli Ben-Yehuda @ 2003-02-21  8:18 UTC (permalink / raw)
  To: Andreas Dilger; +Cc: Jaroslav Kysela, Kernel Mailing List

Bcc: 
Subject: Re: [PATCH] snd_pcm_oss_change_params is a stack offender
Reply-To: 
In-Reply-To: <20030221005852.K1723@schatzie.adilger.int>

On Fri, Feb 21, 2003 at 12:58:52AM -0700, Andreas Dilger wrote:
> On Feb 21, 2003  09:39 +0200, Muli Ben-Yehuda wrote:
> > +static int alloc_param_structs(snd_pcm_hw_params_t** params, 
> > +			       snd_pcm_hw_params_t** sparams,
> > +			       snd_pcm_sw_params_t** sw_params)
> 
> So, it looks like you've changed a large stack user into a leaker of
> memory.  Nowhere is the allocated memory freed, AFAICS, not upon
> successful completion, nor at any of the error exits.

Thanks for spotting. I can only claim not having woken up yet. 

Here's a fixed patch, which frees the allocations properly. I didn't
want to make more than the minimal changes necessary, but if it's ok
with the maintainer, it should be switched to the common "goto style",
and something should be done about those snd_asserts. Jaroslav, ok to
rewrite?

#	sound/core/oss/pcm_oss.c	1.20    -> 1.22   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/02/21	mulix@alhambra.mulix.org	1.1007
# snd_pcm_oss_change_params was a stack offender, having three large 
# structs on the stack. Allocate those structs on the heap and change
# the code accordingly. 
# --------------------------------------------
# 03/02/21	mulix@alhambra.mulix.org	1.1008
# This time, also free the memory :-((
# Thanks to Andreas Dilger for spotting.
# --------------------------------------------
#
diff -Nru a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
--- a/sound/core/oss/pcm_oss.c	Fri Feb 21 10:15:10 2003
+++ b/sound/core/oss/pcm_oss.c	Fri Feb 21 10:15:10 2003
@@ -291,11 +291,58 @@
 	return snd_pcm_hw_param_near(substream, params, SNDRV_PCM_HW_PARAM_RATE, best_rate, 0);
 }
 
+static int alloc_param_structs(snd_pcm_hw_params_t** params, 
+			       snd_pcm_hw_params_t** sparams,
+			       snd_pcm_sw_params_t** sw_params)
+{
+	snd_pcm_hw_params_t* hwp; 
+	snd_pcm_sw_params_t* swp; 
+
+	if (!(hwp = kmalloc(sizeof(*hwp), GFP_KERNEL)))
+		goto out; 
+
+	memset(hwp, 0, sizeof(*hwp)); 
+	*params = hwp; 
+
+	if (!(hwp = kmalloc(sizeof(*hwp), GFP_KERNEL)))
+		goto free_params; 
+
+	memset(hwp, 0, sizeof(*hwp)); 
+	*sparams = hwp; 
+
+	if (!(swp = kmalloc(sizeof(*swp), GFP_KERNEL)))
+		goto free_sparams; 
+
+	memset(swp, 0, sizeof(*swp)); 
+	*sw_params = swp; 
+	
+	return 0; 
+
+ free_sparams:
+	kfree(*sparams); 
+	*sparams = NULL; 
+
+ free_params:
+	kfree(*params); 
+	*params = NULL; 
+
+ out: 
+	return -ENOMEM; 
+}
+
+static void free_param_structs(snd_pcm_hw_params_t* params, snd_pcm_hw_params_t* sparams,
+			       snd_pcm_sw_params_t* sw_params)
+{
+	kfree(params); 
+	kfree(sparams); 
+	kfree(sw_params); 
+}
+
 static int snd_pcm_oss_change_params(snd_pcm_substream_t *substream)
 {
 	snd_pcm_runtime_t *runtime = substream->runtime;
-	snd_pcm_hw_params_t params, sparams;
-	snd_pcm_sw_params_t sw_params;
+	snd_pcm_hw_params_t *params, *sparams;
+	snd_pcm_sw_params_t *sw_params;
 	ssize_t oss_buffer_size, oss_period_size;
 	size_t oss_frame_size;
 	int err;
@@ -311,9 +358,14 @@
 		direct = (setup != NULL && setup->direct);
 	}
 
-	_snd_pcm_hw_params_any(&sparams);
-	_snd_pcm_hw_param_setinteger(&sparams, SNDRV_PCM_HW_PARAM_PERIODS);
-	_snd_pcm_hw_param_min(&sparams, SNDRV_PCM_HW_PARAM_PERIODS, 2, 0);
+	if ((err = alloc_param_structs(&params, &sparams, &sw_params))) {
+		snd_printd("out of memory\n"); 
+		return err; 
+	}
+
+	_snd_pcm_hw_params_any(sparams);
+	_snd_pcm_hw_param_setinteger(sparams, SNDRV_PCM_HW_PARAM_PERIODS);
+	_snd_pcm_hw_param_min(sparams, SNDRV_PCM_HW_PARAM_PERIODS, 2, 0);
 	snd_mask_none(&mask);
 	if (atomic_read(&runtime->mmap_count))
 		snd_mask_set(&mask, SNDRV_PCM_ACCESS_MMAP_INTERLEAVED);
@@ -322,17 +374,18 @@
 		if (!direct)
 			snd_mask_set(&mask, SNDRV_PCM_ACCESS_RW_NONINTERLEAVED);
 	}
-	err = snd_pcm_hw_param_mask(substream, &sparams, SNDRV_PCM_HW_PARAM_ACCESS, &mask);
+	err = snd_pcm_hw_param_mask(substream, sparams, SNDRV_PCM_HW_PARAM_ACCESS, &mask);
 	if (err < 0) {
+		free_param_structs(params, sparams, sw_params); 
 		snd_printd("No usable accesses\n");
 		return -EINVAL;
 	}
-	choose_rate(substream, &sparams, runtime->oss.rate);
-	snd_pcm_hw_param_near(substream, &sparams, SNDRV_PCM_HW_PARAM_CHANNELS, runtime->oss.channels, 0);
+	choose_rate(substream, sparams, runtime->oss.rate);
+	snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_CHANNELS, runtime->oss.channels, 0);
 
 	format = snd_pcm_oss_format_from(runtime->oss.format);
 
-	sformat_mask = *hw_param_mask(&sparams, SNDRV_PCM_HW_PARAM_FORMAT);
+	sformat_mask = *hw_param_mask(sparams, SNDRV_PCM_HW_PARAM_FORMAT);
 	if (direct)
 		sformat = format;
 	else
@@ -345,50 +398,53 @@
 				break;
 		}
 		if (sformat > SNDRV_PCM_FORMAT_LAST) {
+			free_param_structs(params, sparams, sw_params); 
 			snd_printd("Cannot find a format!!!\n");
 			return -EINVAL;
 		}
 	}
-	err = _snd_pcm_hw_param_set(&sparams, SNDRV_PCM_HW_PARAM_FORMAT, sformat, 0);
-	snd_assert(err >= 0, return err);
+	err = _snd_pcm_hw_param_set(sparams, SNDRV_PCM_HW_PARAM_FORMAT, sformat, 0);
+	snd_assert(err >= 0, {free_param_structs(params, sparams, sw_params); return err});
 
 	if (direct) {
-		params = sparams;
+		memcpy(params, sparams, sizeof(*params)); 
 	} else {
-		_snd_pcm_hw_params_any(&params);
-		_snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_ACCESS,
+		_snd_pcm_hw_params_any(params);
+		_snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_ACCESS,
 				      SNDRV_PCM_ACCESS_RW_INTERLEAVED, 0);
-		_snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_FORMAT,
+		_snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_FORMAT,
 				      snd_pcm_oss_format_from(runtime->oss.format), 0);
-		_snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_CHANNELS,
+		_snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_CHANNELS,
 				      runtime->oss.channels, 0);
-		_snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_RATE,
+		_snd_pcm_hw_param_set(params, SNDRV_PCM_HW_PARAM_RATE,
 				      runtime->oss.rate, 0);
 		pdprintf("client: access = %i, format = %i, channels = %i, rate = %i\n",
-			 params_access(&params), params_format(&params),
-			 params_channels(&params), params_rate(&params));
+			 params_access(params), params_format(params),
+			 params_channels(params), params_rate(params));
 	}
 	pdprintf("slave: access = %i, format = %i, channels = %i, rate = %i\n",
-		 params_access(&sparams), params_format(&sparams),
-		 params_channels(&sparams), params_rate(&sparams));
+		 params_access(sparams), params_format(sparams),
+		 params_channels(sparams), params_rate(sparams));
 
-	oss_frame_size = snd_pcm_format_physical_width(params_format(&params)) *
-			 params_channels(&params) / 8;
+	oss_frame_size = snd_pcm_format_physical_width(params_format(params)) *
+			 params_channels(params) / 8;
 
 	snd_pcm_oss_plugin_clear(substream);
 	if (!direct) {
 		/* add necessary plugins */
 		snd_pcm_oss_plugin_clear(substream);
 		if ((err = snd_pcm_plug_format_plugins(substream,
-						       &params, 
-						       &sparams)) < 0) {
+						       params, 
+						       sparams)) < 0) {
+			free_param_structs(params, sparams, sw_params); 
 			snd_printd("snd_pcm_plug_format_plugins failed: %i\n", err);
 			snd_pcm_oss_plugin_clear(substream);
 			return err;
 		}
 		if (runtime->oss.plugin_first) {
 			snd_pcm_plugin_t *plugin;
-			if ((err = snd_pcm_plugin_build_io(substream, &sparams, &plugin)) < 0) {
+			if ((err = snd_pcm_plugin_build_io(substream, sparams, &plugin)) < 0) {
+				free_param_structs(params, sparams, sw_params); 
 				snd_printd("snd_pcm_plugin_build_io failed: %i\n", err);
 				snd_pcm_oss_plugin_clear(substream);
 				return err;
@@ -399,67 +455,73 @@
 				err = snd_pcm_plugin_insert(plugin);
 			}
 			if (err < 0) {
+				free_param_structs(params, sparams, sw_params); 
 				snd_pcm_oss_plugin_clear(substream);
 				return err;
 			}
 		}
 	}
 
-	err = snd_pcm_oss_period_size(substream, &params, &sparams);
-	if (err < 0)
+	err = snd_pcm_oss_period_size(substream, params, sparams);
+	if (err < 0) {
+		free_param_structs(params, sparams, sw_params); 
 		return err;
+	}
 
 	n = snd_pcm_plug_slave_size(substream, runtime->oss.period_bytes / oss_frame_size);
-	err = snd_pcm_hw_param_near(substream, &sparams, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, n, 0);
-	snd_assert(err >= 0, return err);
+	err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIOD_SIZE, n, 0);
+	snd_assert(err >= 0, {free_param_structs(params, sparams, sw_params); return err});
 
-	err = snd_pcm_hw_param_near(substream, &sparams, SNDRV_PCM_HW_PARAM_PERIODS,
+	err = snd_pcm_hw_param_near(substream, sparams, SNDRV_PCM_HW_PARAM_PERIODS,
 				     runtime->oss.periods, 0);
-	snd_assert(err >= 0, return err);
+	snd_assert(err >= 0, {free_param_structs(params, sparams, sw_params); return err});
 
 	snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, 0);
 
-	if ((err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_HW_PARAMS, &sparams)) < 0) {
+	if ((err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_HW_PARAMS, sparams)) < 0) {
+		free_param_structs(params, sparams, sw_params); 
 		snd_printd("HW_PARAMS failed: %i\n", err);
 		return err;
 	}
 
-	memset(&sw_params, 0, sizeof(sw_params));
 	if (runtime->oss.trigger) {
-		sw_params.start_threshold = 1;
+		sw_params->start_threshold = 1;
 	} else {
-		sw_params.start_threshold = runtime->boundary;
+		sw_params->start_threshold = runtime->boundary;
 	}
 	if (atomic_read(&runtime->mmap_count))
-		sw_params.stop_threshold = runtime->boundary;
+		sw_params->stop_threshold = runtime->boundary;
 	else
-		sw_params.stop_threshold = runtime->buffer_size;
-	sw_params.tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
-	sw_params.period_step = 1;
-	sw_params.sleep_min = 0;
-	sw_params.avail_min = runtime->period_size;
-	sw_params.xfer_align = 1;
-	sw_params.silence_threshold = 0;
-	sw_params.silence_size = 0;
+		sw_params->stop_threshold = runtime->buffer_size;
+	sw_params->tstamp_mode = SNDRV_PCM_TSTAMP_NONE;
+	sw_params->period_step = 1;
+	sw_params->sleep_min = 0;
+	sw_params->avail_min = runtime->period_size;
+	sw_params->xfer_align = 1;
+	sw_params->silence_threshold = 0;
+	sw_params->silence_size = 0;
 
-	if ((err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_SW_PARAMS, &sw_params)) < 0) {
+	if ((err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_SW_PARAMS, sw_params)) < 0) {
+		free_param_structs(params, sparams, sw_params); 
 		snd_printd("SW_PARAMS failed: %i\n", err);
 		return err;
 	}
 	runtime->control->avail_min = runtime->period_size;
 
-	runtime->oss.periods = params_periods(&sparams);
-	oss_period_size = snd_pcm_plug_client_size(substream, params_period_size(&sparams));
-	snd_assert(oss_period_size >= 0, return -EINVAL);
+	runtime->oss.periods = params_periods(sparams);
+	oss_period_size = snd_pcm_plug_client_size(substream, params_period_size(sparams));
+	snd_assert(oss_period_size >= 0, {free_param_structs(params, sparams, sw_params); return -EINVAL});
 	if (runtime->oss.plugin_first) {
 		err = snd_pcm_plug_alloc(substream, oss_period_size);
-		if (err < 0)
+		if (err < 0) {
+			free_param_structs(params, sparams, sw_params); 
 			return err;
+		}
 	}
 	oss_period_size *= oss_frame_size;
 
 	oss_buffer_size = oss_period_size * runtime->oss.periods;
-	snd_assert(oss_buffer_size >= 0, return -EINVAL);
+	snd_assert(oss_buffer_size >= 0, {free_param_structs(params, sparams, sw_params); return -EINVAL});
 
 	runtime->oss.period_bytes = oss_period_size;
 	runtime->oss.buffer_bytes = oss_buffer_size;
@@ -468,12 +530,12 @@
 		 runtime->oss.period_bytes,
 		 runtime->oss.buffer_bytes);
 	pdprintf("slave: period_size = %i, buffer_size = %i\n",
-		 params_period_size(&sparams),
-		 params_buffer_size(&sparams));
+		 params_period_size(sparams),
+		 params_buffer_size(sparams));
 
-	runtime->oss.format = snd_pcm_oss_format_to(params_format(&params));
-	runtime->oss.channels = params_channels(&params);
-	runtime->oss.rate = params_rate(&params);
+	runtime->oss.format = snd_pcm_oss_format_to(params_format(params));
+	runtime->oss.channels = params_channels(params);
+	runtime->oss.rate = params_rate(params);
 
 	runtime->oss.params = 0;
 	runtime->oss.prepare = 1;
@@ -483,6 +545,8 @@
 	runtime->oss.buffer_used = 0;
 	if (runtime->dma_area)
 		snd_pcm_format_set_silence(runtime->format, runtime->dma_area, bytes_to_samples(runtime, runtime->dma_bytes));
+
+	free_param_structs(params, sparams, sw_params); 
 	return 0;
 }
 

-- 
Muli Ben-Yehuda
http://www.mulix.org


^ permalink raw reply	[flat|nested] 2+ messages in thread

* a
  2004-05-15 22:18 Problems with radeonfb Sven Wilhelm
@ 2004-05-16 15:33 ` Kronos
  0 siblings, 0 replies; 2+ messages in thread
From: Kronos @ 2004-05-16 15:33 UTC (permalink / raw)
  To: linux-kernel; +Cc: Sven Wilhelm

Sven Wilhelm <wilhelm@icecrash.com> ha scritto:
> Hi list,
> 
> I have problems with the radeonfb on 2.6.6 and also the older 2.6er 
> releases.
[cut]
> radeonfb: Invalid ROM signature 0 should be 0xaa55
> radeonfb: Retreived PLL infos from BIOS
> radeonfb: Reference=27.00 MHz (RefDiv=12) Memory=250.00 Mhz, 
> System=166.00 MHz
> Non-DDC laptop panel detected
> radeonfb: Monitor 1 type LCD found
> radeonfb: Monitor 2 type no found
> radeonfb: panel ID string: CPT CLAA150PA01
> radeonfb: detected LVDS panel size from BIOS: 1400x1050
> radeondb: BIOS provided dividers will be used
> radeonfb: Power Management enabled for Mobility chipsets
> radeonfb: ATI Radeon LW  DDR SGRAM 64 MB
> kobject_register failed for radeonfb (-17)
> Call Trace:
>  [<c0229982>] kobject_register+0x57/0x59
>  [<c0279e08>] bus_add_driver+0x4a/0x9d
>  [<c027a225>] driver_register+0x2f/0x33
>  [<c02332a2>] pci_create_newid_file+0x27/0x29
>  [<c02336a8>] pci_register_driver+0x5c/0x84
>  [<c0430737>] radeonfb_old_init+0xf/0x1d
>  [<c04305bb>] fbmem_init+0x9d/0xe8
>  [<c042cb28>] chr_dev_init+0x80/0x9e
>  [<c041a78e>] do_initcalls+0x28/0xb4
>  [<c012904a>] init_workqueues+0x17/0x31
>  [<c01002b4>] init+0x0/0x150
>  [<c01002ec>] init+0x38/0x150
>  [<c0104258>] kernel_thread_helper+0x0/0xb
>  [<c010425d>] kernel_thread_helper+0x5/0xb

You compiled in both radeon drivers. The old driver is complaining that
the PCI device is already taken by something else. Use only one driver.

Luca
-- 
Home: http://kronoz.cjb.net
Alcuni pensano che io sia una persona orribile, ma non e` vero. Ho il
cuore di un ragazzino - in un vaso sulla scrivania.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2004-05-16 15:33 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-02-21  8:18 a Muli Ben-Yehuda
2004-05-15 22:18 Problems with radeonfb Sven Wilhelm
2004-05-16 15:33 ` a Kronos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).