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

[Marek Chalupa]

On 9 December 2014 at 17:49, Derek Foreman <derekf at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141210/3fba77ff/attachment-0001.html>