Re: [PATCH 55/83] builtin/apply: make parse_single_patch() return -1 on error

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 1, 2016, 19:32 UTC
Message-ID: <CAPig+cSTf+9GJ9r4VZ9A98vQyyzqdGQFbihw2aAtB9sHM4tFCQ@mail.gmail.com>
In-Reply-To: <1461504863-15946-56-git-send-email-chriscool@tuxfamily.org>

On Sun, Apr 24, 2016 at 9:33 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 12 quoted lines

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -1802,8 +1806,10 @@ static int parse_single_patch(struct apply_state *state,
>                 fragment = xcalloc(1, sizeof(*fragment));
>                 fragment->linenr = state->linenr;
>                 len = parse_fragment(state, line, size, patch, fragment);
> -               if (len <= 0)
> -                       die(_("corrupt patch at line %d"), state->linenr);
> +               if (len <= 0) {
> +                       free(fragment);
> +                       return error(_("corrupt patch at line %d"), state->linenr);

No fragment leak here, okay.

Show 13 quoted lines

> +               }
>                 fragment->patch = line;
>                 fragment->size = len;
>                 oldlines += fragment->oldlines;
> @@ -1839,9 +1845,9 @@ static int parse_single_patch(struct apply_state *state,
>                 patch->is_delete = 0;
>
>         if (0 < patch->is_new && oldlines)
> -               die(_("new file %s depends on old contents"), patch->new_name);
> +               return error(_("new file %s depends on old contents"), patch->new_name);
>         if (0 < patch->is_delete && newlines)
> -               die(_("deleted file %s still has contents"), patch->old_name);
> +               return error(_("deleted file %s still has contents"), patch->old_name);

These two return -1 and don't manually release the fragments, which might seem like a leak, however...

Show 9 quoted lines

>         if (!patch->is_delete && !newlines && context)
>                 fprintf_ln(stderr,
>                            _("** warning: "
> @@ -2143,6 +2149,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
>                                        size - offset - hdrsize,
>                                        patch);
>
> +       if (patchsize < 0)
> +               return -1;

This forwards -1 to its caller, and the caller, apply_patch() does release the fragments when it sees -1, so there does not seem to be any (new) leak. Good.