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

From: Brandon Casey <drafnel@gmail.com>
Date: Aug 24, 2017, 18:29 UTC
Message-ID: <CA+sFfMdYXDt2mgnWq-HQQyBsCqYZ+689BCKEOw7siGjQoUysjg@mail.gmail.com>
In-Reply-To: <xmqqpobkx610.fsf@gitster.mtv.corp.google.com>

On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

> Brandon Casey <drafnel@gmail.com> writes:
>
>> Ah, you probably meant something like this:
>>
>>    const char strbuf_slopbuf = '\0';
>>
>> which gcc will apparently place in the read-only segment.  I did not know that.
>
> Yes but I highly suspect that it would be very compiler dependent
> and not something the language lawyers would recommend us to rely
> on.

I think compilers may have the option of placing variables that are explicitly initialized to zero in the bss segment too, in addition to those that are not explicitly initialized. So I agree that no one should write code that depends on their variables being placed in one segment or the other, but I could see someone using this behavior as an additional safety check; kind of a free assert, aside from the additional space in the .rodata segment.

> My response was primarily to answer "why?" with "because we did not
> bother".  The above is a mere tangent, i.e. "multiple copies of
> empty strings is a horrible implementation (and there would be a way
> to do it with a single instance)".

Merely adding const to our current strbuf_slopbuf declaration does not buy us anything since it will be allocated in r/w memory. i.e. it would still be possible to modify it via the buf member of strbuf. So you can't just do this:

   const char strbuf_slopbuf[1];

That's pretty much equivalent to what we currently have since it only restricts modifying the contents of strbuf_slopbuf directly through the strbuf_slopbuf variable, but it does not restrict modifying it through a pointer to that object.

Until yesterday, I was under the impression that the only way to access data in the rodata segment was through a constant literal. So my initial thought was that we could do something like:

   const char * const strbuf_slopbuf = "";

..but that variable cannot be used in a static assignment like:

   struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};

So it seemed like our only option was to use a literal "" everywhere instead of a slopbuf variable _if_ we wanted to have the guarantee that our "slopbuf" could not be modified.

But what I learned yesterday, is that at least gcc/clang will place the entire variable in the rodata segment if the variable is both marked const _and_ initialized.

i.e. this will be allocated in the .rodata segment:

   const char strbuf_slopbuf[1] = "";

Show 8 quoted lines

>>    #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) &strbuf_slopbuf }
>>
>> respectively.  Yeah, that's definitely preferable to a macro.
>> Something similar could be done in object.c.
>
> What is the main objective for doing this change?  The "make sure we
> do not write into that slopbuf" assert() bothers you and you want to
> replace it with an address in the read-only segment?

Actually nothing about the patch bothers me. The point of that patch is to make sure we don't accidentally modify the slopbuf. I was just looking for a way for the compiler to help out and wondering if there was a reason we didn't attempt to do so in the first place.

I think the main takeaway here is that I learned something yesterday :-) I didn't actually intend to submit a patch for any of this, but if anything useful came out of the discussion I thought Martin may incorporate it into his patch if he wanted to.

-Brandon