[1/3] Use zalloc rather than malloc and manually setting members to 0

Submitted by Bryce Harrington on Nov. 21, 2014, 6:21 a.m.

Details

Message ID 1416550917-14921-1-git-send-email-bryce@osg.samsung.com
State Accepted
Commit de44761a1a51a7fb79f7d239b802fd3f1660f744
Headers show

Not browsing as part of any series.

Commit Message

Bryce Harrington Nov. 21, 2014, 6:21 a.m.
Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
---
 src/clipboard.c     | 5 +++--
 src/screenshooter.c | 9 ++-------
 2 files changed, 5 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/clipboard.c b/src/clipboard.c
index 5a3a02d..dbd8c9b 100644
--- a/src/clipboard.c
+++ b/src/clipboard.c
@@ -214,9 +214,10 @@  clipboard_client_create(struct clipboard_source *source, int fd)
 	struct wl_event_loop *loop =
 		wl_display_get_event_loop(seat->compositor->wl_display);
 
-	client = malloc(sizeof *client);
+	client = zalloc(sizeof *client);
+	if (client == NULL)
+		return;
 
-	client->offset = 0;
 	client->source = source;
 	source->refcount++;
 	client->event_source =
diff --git a/src/screenshooter.c b/src/screenshooter.c
index cafbf10..6246cda 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -481,7 +481,7 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 
 	do_yflip = !!(compositor->capabilities & WESTON_CAP_CAPTURE_YFLIP);
 
-	recorder = malloc(sizeof *recorder);
+	recorder = zalloc(sizeof *recorder);
 	if (recorder == NULL) {
 		weston_log("%s: out of memory\n", __func__);
 		return;
@@ -491,9 +491,6 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 	size = stride * 4 * output->current_mode->height;
 	recorder->frame = zalloc(size);
 	recorder->rect = malloc(size);
-	recorder->total = 0;
-	recorder->count = 0;
-	recorder->destroying = 0;
 	recorder->output = output;
 
 	if ((recorder->frame == NULL) || (recorder->rect == NULL)) {
@@ -502,9 +499,7 @@  weston_recorder_create(struct weston_output *output, const char *filename)
 		return;
 	}
 
-	if (do_yflip)
-		recorder->tmpbuf = NULL;
-	else
+	if (!do_yflip)
 		recorder->tmpbuf = malloc(size);
 
 	header.magic = WCAP_HEADER_MAGIC;

Comments

On 21 November 2014 at 07:21, Bryce Harrington <bryce@osg.samsung.com>
wrote:

> Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> ---
>  src/clipboard.c     | 5 +++--
>  src/screenshooter.c | 9 ++-------
>  2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/src/clipboard.c b/src/clipboard.c
> index 5a3a02d..dbd8c9b 100644
> --- a/src/clipboard.c
> +++ b/src/clipboard.c
> @@ -214,9 +214,10 @@ clipboard_client_create(struct clipboard_source
> *source, int fd)
>         struct wl_event_loop *loop =
>                 wl_display_get_event_loop(seat->compositor->wl_display);
>
> -       client = malloc(sizeof *client);
> +       client = zalloc(sizeof *client);
> +       if (client == NULL)
> +               return;
>
> -       client->offset = 0;
>         client->source = source;
>         source->refcount++;
>         client->event_source =
> diff --git a/src/screenshooter.c b/src/screenshooter.c
> index cafbf10..6246cda 100644
> --- a/src/screenshooter.c
> +++ b/src/screenshooter.c
> @@ -481,7 +481,7 @@ weston_recorder_create(struct weston_output *output,
> const char *filename)
>
>         do_yflip = !!(compositor->capabilities & WESTON_CAP_CAPTURE_YFLIP);
>
> -       recorder = malloc(sizeof *recorder);
> +       recorder = zalloc(sizeof *recorder);
>         if (recorder == NULL) {
>                 weston_log("%s: out of memory\n", __func__);
>                 return;
> @@ -491,9 +491,6 @@ weston_recorder_create(struct weston_output *output,
> const char *filename)
>         size = stride * 4 * output->current_mode->height;
>         recorder->frame = zalloc(size);
>         recorder->rect = malloc(size);
> -       recorder->total = 0;
> -       recorder->count = 0;
> -       recorder->destroying = 0;
>         recorder->output = output;
>
>         if ((recorder->frame == NULL) || (recorder->rect == NULL)) {
> @@ -502,9 +499,7 @@ weston_recorder_create(struct weston_output *output,
> const char *filename)
>                 return;
>         }
>
> -       if (do_yflip)
> -               recorder->tmpbuf = NULL;
> -       else
> +       if (!do_yflip)
>                 recorder->tmpbuf = malloc(size);
>

Here you don't check the malloc's result. Also in weston_recorder_destroy
there's no check that tmpbuf was allocated before freeing that memory.


>
>         header.magic = WCAP_HEADER_MAGIC;
> --
> 1.9.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>

Thanks,
Marek
On Wed, Nov 26, 2014 at 03:33:16PM +0100, Marek Chalupa wrote:
> On 21 November 2014 at 07:21, Bryce Harrington <bryce@osg.samsung.com>
> wrote:
> 
> > Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> >
> > -       if (do_yflip)
> > -               recorder->tmpbuf = NULL;
> > -       else
> > +       if (!do_yflip)
> >                 recorder->tmpbuf = malloc(size);
> >
> 
> Here you don't check the malloc's result.

Good point.  The original code didn't either, but
weston_recorder_frame_notify() looks like it assumes tmpbuf is valid and
would crash there otherwise.  I can add this.

> Also in weston_recorder_destroy
> there's no check that tmpbuf was allocated before freeing that memory.

Actually it's unnecessary to check for NULL before calling free().
free(NULL) is a no-op as per `man 3 malloc`.

Thanks for the review!

Bryce
Hi,

On 26 November 2014 at 22:49, Bryce Harrington <bryce@osg.samsung.com>
wrote:

> On Wed, Nov 26, 2014 at 03:33:16PM +0100, Marek Chalupa wrote:
> > On 21 November 2014 at 07:21, Bryce Harrington <bryce@osg.samsung.com>
> > wrote:
> >
> > > Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
> > >
> > > -       if (do_yflip)
> > > -               recorder->tmpbuf = NULL;
> > > -       else
> > > +       if (!do_yflip)
> > >                 recorder->tmpbuf = malloc(size);
> > >
> >
> > Here you don't check the malloc's result.
>
> Good point.  The original code didn't either, but
> weston_recorder_frame_notify() looks like it assumes tmpbuf is valid and
> would crash there otherwise.  I can add this.
>

I found out that I have add this check when I was testing your patches, so
I sent it to list.
I hope you don't mind that I took you the 'work' here :)


>
> > Also in weston_recorder_destroy
> > there's no check that tmpbuf was allocated before freeing that memory.
>
> Actually it's unnecessary to check for NULL before calling free().
> free(NULL) is a no-op as per `man 3 malloc`.
>
> Thanks for the review!
>
> Bryce
>
>
Thanks,
Marek