Re: [PATCH 02/26] mailinfo: fix for off-by-one error in boundary stack

[Stefan Beller]

On Tue, Oct 13, 2015 at 4:16 PM, Junio C Hamano  wrote:
> We pre-increment the pointer that we will use to store something at,
> so the pointer is already beyond the end of the array if it points
> at content[MAX_BOUNDARIES].
>
> Signed-off-by: Junio C Hamano 
> ---
>
>  As always, I am very bad at checking and fixing off-by-one errors.
>  A few extra sets of eyeballs are very much appreciated.

some relevant lines from the file:
#define MAX_BOUNDARIES 5
static struct strbuf *content[MAX_BOUNDARIES];
static struct strbuf **content_top = content;

content has slots 0..4, so checking content[5] is out of range,
but was allowed in the original code.

So this patch looks good to me, though looking at
289796dd29dd656734cfd59b657deb943a71cf6a,
makes me wonder if we forget the other spot where it's
decremented in that patch.

Also looking at the patch, makes we wonder if we rather want to change
-static struct strbuf *content[MAX_BOUNDARIES];
+static struct strbuf *content[MAX_BOUNDARIES + 1];

instead? That would actually increase the allocated memory,
and allow to write one more content line, but reading the code is
easier as we don't need to reason about > or >=.

Another way would be to rewrite this part to use an index instead
of content_top being a pointer. And the index could be compared
to strictly less than MAX_BOUNDARIES.

> ---
>  builtin/mailinfo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 330bef4..2742d0d 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -185,7 +185,7 @@ static void handle_content_type(struct strbuf *line)
>
> if (slurp_attr(line->buf, "boundary=", boundary)) {
> strbuf_insert(boundary, 0, "--", 2);
> -   if (++content_top > [MAX_BOUNDARIES]) {
> +   if (++content_top >= [MAX_BOUNDARIES]) {
> fprintf(stderr, "Too many boundaries to handle\n");
> exit(1);
> }
> --
> 2.6.1-320-g86a1181
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/26] mailinfo: fix for off-by-one error in boundary stack

[Junio C Hamano]

I'll be sending out v2 very soon, so you might want to hold off for
a few minutes.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/26] mailinfo: fix for off-by-one error in boundary stack

[Junio C Hamano]

We pre-increment the pointer that we will use to store something at,
so the pointer is already beyond the end of the array if it points
at content[MAX_BOUNDARIES].

Signed-off-by: Junio C Hamano 
---

 As always, I am very bad at checking and fixing off-by-one errors.
 A few extra sets of eyeballs are very much appreciated.
---
 builtin/mailinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 330bef4..2742d0d 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -185,7 +185,7 @@ static void handle_content_type(struct strbuf *line)
 
if (slurp_attr(line->buf, "boundary=", boundary)) {
strbuf_insert(boundary, 0, "--", 2);
-   if (++content_top > [MAX_BOUNDARIES]) {
+   if (++content_top >= [MAX_BOUNDARIES]) {
fprintf(stderr, "Too many boundaries to handle\n");
exit(1);
}
-- 
2.6.1-320-g86a1181

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html