[PATCH] Don't try to malloc a zero sized list

[Anton Lundin]

On 11 December, 2014 - Linus Torvalds wrote:

> On Thu, Dec 11, 2014 at 3:31 PM, Anton Lundin <glance at acc.umu.se> wrote:
> > If we tried to copy a divecomputer without samples, we where to malloc a
> > zero sized blob. dives/test15.xml triggered this and it was found with
> > valgrind.
> 
> This is *not* correct.
> 
> First off, allocating a zero-sized area is legal, although it
> might/should return NULL. So the patch doesn't "fix" anything.
> 
> But worse, the patch *breaks* stuff, by now not initializing the
> "d->samples" and "d->sample" fields at all.
> 
> And at least MainTab::acceptChanges() definitely expects
> copy_samples() to properly initialize those fields, because lookie
> here:
> 
>                         free(current_dive->dc.sample);
>                         copy_samples(&displayed_dive.dc, &current_dive->dc);
> 
> notice how it just free'd the "sample" pointer, and your patch now
> does not overwrite it if the new source dc has no samples.
> 
> So NAK. This patch is wrong.
> 

The problem is in the case where malloc actually returns a pointer to a
zero sized memory area. Eg. is_cylinder_used:

	if (dc->sample && event->time.seconds == dc->sample[0].time.seconds)

So if dc->sample is a valid pointer, it follows that pointer, and if the
memory area is zero in size it reads invalid memory.


So, should the right approach here be not to trust malloc returning NULL
here and we explicitly set sample to null if s->samples == 0 ?


--- a/dive.c
+++ b/dive.c
@@ -601,7 +601,14 @@ void copy_samples(struct divecomputer *s, struct divecomputer *d)
 	 	return;
 	int nr = s->samples;
 	d->samples = nr;
-	d->sample = malloc(nr * sizeof(struct sample));
+	// We expect to be able to read the memory in the other end of the pointer
+	// if its a valid pointer, so don't expect malloc() to return NULL for
+	// zero-sized malloc, do it our selfs.
+	if (nr)
+ 		d->sample = malloc(nr * sizeof(struct sample));
+	else
+ 		d->sample = NULL;
+
 	if (d->sample)
 		memcpy(d->sample, s->sample, nr * sizeof(struct sample));
 }



Another thought, taken how the current copy_samples looks, if you where
to call prepare_sample(d) after a copy_samples(s, d), which copied more
than 10 samples, too me it looks like prepare_sample would truncate the
sample list because copy_samples doesn't set dc->alloc_samples.


//Anton


-- 
Anton Lundin	+46702-161604