[weston,2/3] screenshooter: fix various memory handling

Submitted by Marek Chalupa on Dec. 5, 2014, 12:49 p.m.

Details

Message ID 1417783782-12792-2-git-send-email-mchqwerty@gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Marek Chalupa Dec. 5, 2014, 12:49 p.m.
There were unchecked malloc and free of conditionally allocated
memory without checking if the memory was really allocated.
Also simplify error handling in one function.

Signed-off-by: Marek Chalupa <mchqwerty@gmail.com>
---
 src/screenshooter.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/screenshooter.c b/src/screenshooter.c
index 6246cda..b7b1fd0 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -464,8 +464,11 @@  weston_recorder_free(struct weston_recorder *recorder)
 {
 	if (recorder == NULL)
 		return;
+
+	if (recorder->tmpbuf)
+		free(recorder->tmpbuf);
+
 	free(recorder->rect);
-	free(recorder->tmpbuf);
 	free(recorder->frame);
 	free(recorder);
 }
@@ -495,12 +498,16 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 
 	if ((recorder->frame == NULL) || (recorder->rect == NULL)) {
 		weston_log("%s: out of memory\n", __func__);
-		weston_recorder_free(recorder);
-		return;
+		goto err_recorder;
 	}
 
-	if (!do_yflip)
+	if (!do_yflip) {
 		recorder->tmpbuf = malloc(size);
+		if (recorder->tmpbuf == NULL) {
+			weston_log("%s: out of memory\n", __func__);
+			goto err_recorder;
+		}
+	}
 
 	header.magic = WCAP_HEADER_MAGIC;
 
@@ -514,8 +521,7 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 		break;
 	default:
 		weston_log("unknown recorder format\n");
-		weston_recorder_free(recorder);
-		return;
+		goto err_recorder;
 	}
 
 	recorder->fd = open(filename,
@@ -523,8 +529,7 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 
 	if (recorder->fd < 0) {
 		weston_log("problem opening output file %s: %m\n", filename);
-		weston_recorder_free(recorder);
-		return;
+		goto err_recorder;
 	}
 
 	header.width = output->current_mode->width;
@@ -535,6 +540,12 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 	wl_signal_add(&output->frame_signal, &recorder->frame_listener);
 	output->disable_planes++;
 	weston_output_damage(output);
+
+	return;
+
+err_recorder:
+	weston_recorder_free(recorder);
+	return;
 }
 
 static void

Comments

On 05/12/14 06:49 AM, Marek Chalupa wrote:
> There were unchecked malloc and free of conditionally allocated
> memory without checking if the memory was really allocated.
> Also simplify error handling in one function.
> 
> Signed-off-by: Marek Chalupa <mchqwerty@gmail.com>
> ---
>  src/screenshooter.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/src/screenshooter.c b/src/screenshooter.c
> index 6246cda..b7b1fd0 100644
> --- a/src/screenshooter.c
> +++ b/src/screenshooter.c
> @@ -464,8 +464,11 @@ weston_recorder_free(struct weston_recorder *recorder)
>  {
>  	if (recorder == NULL)
>  		return;
> +
> +	if (recorder->tmpbuf)
> +		free(recorder->tmpbuf);
> +
>  	free(recorder->rect);
> -	free(recorder->tmpbuf);

I think recorder->tmpbuf should be NULL if it was never required
(recorder is allocated with zalloc), and free(NULL); is a NOP...

>  	free(recorder->frame);
>  	free(recorder);
>  }
> @@ -495,12 +498,16 @@ weston_recorder_create(struct weston_output *output, const char *filename)
>  
>  	if ((recorder->frame == NULL) || (recorder->rect == NULL)) {
>  		weston_log("%s: out of memory\n", __func__);
> -		weston_recorder_free(recorder);
> -		return;
> +		goto err_recorder;
>  	}
>  
> -	if (!do_yflip)
> +	if (!do_yflip) {
>  		recorder->tmpbuf = malloc(size);
> +		if (recorder->tmpbuf == NULL) {
> +			weston_log("%s: out of memory\n", __func__);
> +			goto err_recorder;
> +		}

Yup, we need this bit.

And I like the goto err_recorder stuff as well, though I guess that's a
matter of preference.

> +	}
>  
>  	header.magic = WCAP_HEADER_MAGIC;
>  
> @@ -514,8 +521,7 @@ weston_recorder_create(struct weston_output *output, const char *filename)
>  		break;
>  	default:
>  		weston_log("unknown recorder format\n");
> -		weston_recorder_free(recorder);
> -		return;
> +		goto err_recorder;
>  	}
>  
>  	recorder->fd = open(filename,
> @@ -523,8 +529,7 @@ weston_recorder_create(struct weston_output *output, const char *filename)
>  
>  	if (recorder->fd < 0) {
>  		weston_log("problem opening output file %s: %m\n", filename);
> -		weston_recorder_free(recorder);
> -		return;
> +		goto err_recorder;
>  	}
>  
>  	header.width = output->current_mode->width;
> @@ -535,6 +540,12 @@ weston_recorder_create(struct weston_output *output, const char *filename)
>  	wl_signal_add(&output->frame_signal, &recorder->frame_listener);
>  	output->disable_planes++;
>  	weston_output_damage(output);
> +
> +	return;
> +
> +err_recorder:
> +	weston_recorder_free(recorder);
> +	return;
>  }
>  
>  static void
>
On 9 December 2014 at 17:49, Derek Foreman <derekf@osg.samsung.com> wrote:

> On 05/12/14 06:49 AM, Marek Chalupa wrote:
> > There were unchecked malloc and free of conditionally allocated
> > memory without checking if the memory was really allocated.
> > Also simplify error handling in one function.
> >
> > Signed-off-by: Marek Chalupa <mchqwerty@gmail.com>
> > ---
> >  src/screenshooter.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/screenshooter.c b/src/screenshooter.c
> > index 6246cda..b7b1fd0 100644
> > --- a/src/screenshooter.c
> > +++ b/src/screenshooter.c
> > @@ -464,8 +464,11 @@ weston_recorder_free(struct weston_recorder
> *recorder)
> >  {
> >       if (recorder == NULL)
> >               return;
> > +
> > +     if (recorder->tmpbuf)
> > +             free(recorder->tmpbuf);
> > +
> >       free(recorder->rect);
> > -     free(recorder->tmpbuf);
>
> I think recorder->tmpbuf should be NULL if it was never required
> (recorder is allocated with zalloc), and free(NULL); is a NOP...
>

Yep, man pages say that free(NULL) is a no-op.
Until now I've thought that NULL is invalid arg for free()... Man keeps
learning :)


>
> >       free(recorder->frame);
> >       free(recorder);
> >  }
> > @@ -495,12 +498,16 @@ weston_recorder_create(struct weston_output
> *output, const char *filename)
> >
> >       if ((recorder->frame == NULL) || (recorder->rect == NULL)) {
> >               weston_log("%s: out of memory\n", __func__);
> > -             weston_recorder_free(recorder);
> > -             return;
> > +             goto err_recorder;
> >       }
> >
> > -     if (!do_yflip)
> > +     if (!do_yflip) {
> >               recorder->tmpbuf = malloc(size);
> > +             if (recorder->tmpbuf == NULL) {
> > +                     weston_log("%s: out of memory\n", __func__);
> > +                     goto err_recorder;
> > +             }
>
> Yup, we need this bit.
>
> And I like the goto err_recorder stuff as well, though I guess that's a
> matter of preference.
>
> > +     }
> >
> >       header.magic = WCAP_HEADER_MAGIC;
> >
> > @@ -514,8 +521,7 @@ weston_recorder_create(struct weston_output *output,
> const char *filename)
> >               break;
> >       default:
> >               weston_log("unknown recorder format\n");
> > -             weston_recorder_free(recorder);
> > -             return;
> > +             goto err_recorder;
> >       }
> >
> >       recorder->fd = open(filename,
> > @@ -523,8 +529,7 @@ weston_recorder_create(struct weston_output *output,
> const char *filename)
> >
> >       if (recorder->fd < 0) {
> >               weston_log("problem opening output file %s: %m\n",
> filename);
> > -             weston_recorder_free(recorder);
> > -             return;
> > +             goto err_recorder;
> >       }
> >
> >       header.width = output->current_mode->width;
> > @@ -535,6 +540,12 @@ weston_recorder_create(struct weston_output
> *output, const char *filename)
> >       wl_signal_add(&output->frame_signal, &recorder->frame_listener);
> >       output->disable_planes++;
> >       weston_output_damage(output);
> > +
> > +     return;
> > +
> > +err_recorder:
> > +     weston_recorder_free(recorder);
> > +     return;
> >  }
> >
> >  static void
> >
>
>
Thanks for reviewing, sending new patch with the free() thing fixed

Marek