Re: [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image()

[René Scharfe]

Am 16.07.2017 um 02:31 schrieb Ramsay Jones:



On 15/07/17 21:20, René Scharfe wrote:

Simplify the code by using the helper macros COPY_ARRAY and MOVE_ARRAY,
which also makes them more robust in the case we copy or move no lines,
as they allow using NULL points in that case, while memcpy(3) and
memmove(3) don't.

Found with Clang's UBSan.

Signed-off-by: Rene Scharfe 
---
I don't know why the rules in contrib/coccinelle/array.cocci didn't
match. :-?

  apply.c | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/apply.c b/apply.c
index f2d599141d..40707ca50c 100644
--- a/apply.c
+++ b/apply.c
@@ -2809,13 +2809,10 @@ static void update_image(struct apply_state *state,
img->line_allocated = img->line;
}
if (preimage_limit != postimage->nr)
-   memmove(img->line + applied_pos + postimage->nr,
-   img->line + applied_pos + preimage_limit,
-   (img->nr - (applied_pos + preimage_limit)) *
-   sizeof(*img->line));
-   memcpy(img->line + applied_pos,
-  postimage->line,
-  postimage->nr * sizeof(*img->line));
+   MOVE_ARRAY(img->line + applied_pos + postimage->nr,
+  img->line + applied_pos + preimage_limit,
+  img->nr - (applied_pos + preimage_limit));
+   COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);


My patch looks like:

-   memcpy(img->line + applied_pos,
-  postimage->line,
-  postimage->nr * sizeof(*img->line));
+   if (postimage->line && postimage->nr)
+   memcpy(img->line + applied_pos,
+  postimage->line,
+  postimage->nr * sizeof(*img->line));

... which I think I prefer (slightly).


Can postimage->line be NULL when postimage->nr is bigger than 0?  What
would that mean?  The only ways to arrive at that point that I an come
up with are bugs (we accidentally set ->line to NULL, or we forgot to
clean ->line).  We'd better notice them early by getting a nice
shrieking segfault.  Adding an assert would work as well.

René

Re: [PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image()

[Ramsay Jones]

On 15/07/17 21:20, René Scharfe wrote:
> Simplify the code by using the helper macros COPY_ARRAY and MOVE_ARRAY,
> which also makes them more robust in the case we copy or move no lines,
> as they allow using NULL points in that case, while memcpy(3) and
> memmove(3) don't.
> 
> Found with Clang's UBSan.
> 
> Signed-off-by: Rene Scharfe 
> ---
> I don't know why the rules in contrib/coccinelle/array.cocci didn't
> match. :-?
> 
>  apply.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/apply.c b/apply.c
> index f2d599141d..40707ca50c 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2809,13 +2809,10 @@ static void update_image(struct apply_state *state,
>   img->line_allocated = img->line;
>   }
>   if (preimage_limit != postimage->nr)
> - memmove(img->line + applied_pos + postimage->nr,
> - img->line + applied_pos + preimage_limit,
> - (img->nr - (applied_pos + preimage_limit)) *
> - sizeof(*img->line));
> - memcpy(img->line + applied_pos,
> -postimage->line,
> -postimage->nr * sizeof(*img->line));
> + MOVE_ARRAY(img->line + applied_pos + postimage->nr,
> +img->line + applied_pos + preimage_limit,
> +img->nr - (applied_pos + preimage_limit));
> + COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);

My patch looks like:

-   memcpy(img->line + applied_pos,
-  postimage->line,
-  postimage->nr * sizeof(*img->line));
+   if (postimage->line && postimage->nr)
+   memcpy(img->line + applied_pos,
+  postimage->line,
+  postimage->nr * sizeof(*img->line));

... which I think I prefer (slightly).


ATB,
Ramsay Jones

>   if (!state->allow_overlap)
>   for (i = 0; i < postimage->nr; i++)
>   img->line[applied_pos + i].flag |= LINE_PATCHED;
> 

[PATCH 3/2] apply: use COPY_ARRAY and MOVE_ARRAY in update_image()

[René Scharfe]

Simplify the code by using the helper macros COPY_ARRAY and MOVE_ARRAY,
which also makes them more robust in the case we copy or move no lines,
as they allow using NULL points in that case, while memcpy(3) and
memmove(3) don't.

Found with Clang's UBSan.

Signed-off-by: Rene Scharfe 
---
I don't know why the rules in contrib/coccinelle/array.cocci didn't
match. :-?

 apply.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/apply.c b/apply.c
index f2d599141d..40707ca50c 100644
--- a/apply.c
+++ b/apply.c
@@ -2809,13 +2809,10 @@ static void update_image(struct apply_state *state,
img->line_allocated = img->line;
}
if (preimage_limit != postimage->nr)
-   memmove(img->line + applied_pos + postimage->nr,
-   img->line + applied_pos + preimage_limit,
-   (img->nr - (applied_pos + preimage_limit)) *
-   sizeof(*img->line));
-   memcpy(img->line + applied_pos,
-  postimage->line,
-  postimage->nr * sizeof(*img->line));
+   MOVE_ARRAY(img->line + applied_pos + postimage->nr,
+  img->line + applied_pos + preimage_limit,
+  img->nr - (applied_pos + preimage_limit));
+   COPY_ARRAY(img->line + applied_pos, postimage->line, postimage->nr);
if (!state->allow_overlap)
for (i = 0; i < postimage->nr; i++)
img->line[applied_pos + i].flag |= LINE_PATCHED;
-- 
2.13.3