bug#36431: Crash in marker.c:337

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: wl@gnu.org,  36431@debbugs.gnu.org
> Date: Tue, 02 Jul 2019 17:00:21 -0400
> 
> > It says that (a) CODING->src_pos is the negative of the offset from
> > GPT of where the bytes are in the gap
> 
> Yes, I think this is actually wrong.

You are right.

> E.e. decode_coding_gap does:
> 
>     coding->src_chars = chars;
>     [...]
>     coding->src_pos = -chars;
> 
> so nowhere does it account for unspecified number of bytes between the
> beginning of the gap and the beginning of the source text.
> Here, `src_pos` is the offset from the end of the gap.

Yes, the clear evidence is in coding_set_source:

      if (coding->src_pos < 0)
        coding->source = BUF_GAP_END_ADDR (buf) + coding->src_pos_byte;

(Note that it actually only uses the byte offset's numerical value.  I
couldn't find any place where it actually uses the character offset in
src_pos, it only checks its sign.)

> > As for why this was designed like that -- where else did you see
> > comments in Emacs that answer this kind of questions?
> 
> There are such design comments at various places.
> Usually added after the fact, sometimes added as part of
> a commit-reversal to make sure someone else doesn't fall into the same
> trap ;-)

Interesting.  Can you point me to a couple of such design comments?

>     If CODING->src_object is a buffer, it must be the current buffer.
>     In this case, if CODING->src_pos is positive, it is a position of
>     the source text in the buffer, otherwise, the source text is in the
> -   gap area of the buffer, and CODING->src_pos specifies the offset of
> -   the text from GPT (which must be the same as PT).  If this is the
> -   same buffer as CODING->dst_object, CODING->src_pos must be
> -   negative.
> +   gap area of the buffer, and CODING->src_pos specifies the
> +   offset of the text from the end of the gap (which must be at PT).

The "which must be at PT" part is ambiguous.  I suggest to say
explicitly that the gap must be at PT (although decode-coding doesn't
really blindly assume that, as you can see from its calls to
move_gap_both).

> +   If this is the same buffer as CODING->dst_object, CODING->src_pos must
> +   be negative.

I don't see the condition of the same buffer in the code.  What did I
miss?

> +   When the text is taken from the gap, it can't be at the beginning of
> +   the gap so that we can produce the decoded text at the beginning of
> +   the gap: this way, as the output grows, the input shrinks, so we only
> +   need to allocate enough space for `max(IN, OUT)` instead of `IN + OUT`.

I think this should also tell that decoding in this setup takes bytes
from encoded text at the end of the gap and inserts the decoded text
starting at PT, which is the same as the beginning of the gap.
Without saying that, the above paragraph might not be as clear as it
should be.