Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

From: Brandon Casey <drafnel@gmail.com>
Date: Aug 23, 2017, 20:37 UTC
Message-ID: <CA+sFfMdXv+nqpXmwfLTHtkRLuGkAEAwWXZCvOryVZ=aLb_UmbA@mail.gmail.com>
In-Reply-To: <dccd3e75fcd1b2de93263e8373a3b4cd5da0dd32.1503323391.git.martin.agren@gmail.com>

On Mon, Aug 21, 2017 at 10:43 AM, Martin Ågren <martin.agren@gmail.com> wrote:

Show 6 quoted lines

> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:

<snip very well written commit message>

Show 19 quoted lines

> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
>         if (len > (sb->alloc ? sb->alloc - 1 : 0))
>                 die("BUG: strbuf_setlen() beyond buffer");
>         sb->len = len;
> -       sb->buf[len] = '\0';
> +       if (sb->buf != strbuf_slopbuf)
> +               sb->buf[len] = '\0';
> +       else
> +               assert(!strbuf_slopbuf[0]);
>  }
>
>  /**
> --
> 2.14.1.151.gdfeca7a7e
>

I know this must have been discussed before and/or I'm having a neuron misfire, but is there any reason why we didn't just make slopbuf a macro for ""?

i.e.

   #define strbuf_slopbuf ""

That way it should point to readonly memory and any attempt to assign to it should produce a crash.

One benefit that I can think of for making strbuf_slopbuf be a real variable is that we can then compare the pointer stored in the strbuf to the strbuf_slopbuf address to detect whether the strbuf held the slopbuf. With the static string macro, each execution unit may get it's own instance of the empty string. But, before this patch, we don't actually seem to be doing that anywhere and instead rely on the value of alloc being accurate to determine whether the strbuf contains the slopbuf or not.

So is there any reason why didn't do something like the following in the first place?

diff --git a/strbuf.h b/strbuf.h
index e705b94..fcca618 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,7 +67,7 @@ struct strbuf {
        char *buf;
 };

-extern char strbuf_slopbuf[];
+#define strbuf_slopbuf ""
 #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }

 /**
@@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf
*sb, size_t len)
        if (len > (sb->alloc ? sb->alloc - 1 : 0))
                die("BUG: strbuf_setlen() beyond buffer");
        sb->len = len;
-       sb->buf[len] = '\0';
+       if (sb->alloc) {
+               sb->buf[len] = '\0';
+       }
 }

-Brandon