bug#70122: 29.3.50; transpose-regions can crash Emacs

[Eli Zaretskii]

> From: Braun Gábor <braungb88@gmail.com>
> Cc: 70122@debbugs.gnu.org
> Date: Wed, 03 Apr 2024 20:52:47 +0200
> 
> I've attached a new self-contained patch based on yours,
> thank you for coming up with it.
> 
> In my opinion, one of the problems is really as you said
> that len_mid (length in bytes)
> is uses where length of characters is expected.
> The patch contains an additional such case,
> and replaces (start1 + len1) in your patch with the equivalent
> but shorter end1.
> 
> The other problem is that the branch len1_byte == len2_byte
> assumes len1 == len2 in several places:
> undo records, positions after the transposition,
> and the least obvious one is that even intervals between the 
> region seem to need adjustment of positions.

Can we lift that restriction by augmenting the len1_byte == len2_byte
branch so that the len1 == len2 condition is not needed?

> (I don't know enough of intervals to understand it, but had failed 
> tests with text properties at wrong places.)

Intervals work with character positions, that's all.  Any call to
functions that examine or modify intervals should use character
positions, not byte positions, and accordingly length in terms of
characters, not bytes.  Is this sufficient for you to come up with
changes to remove the len1 == len2 restriction from that branch?

> Anyway, I've just added len1 == len2 as a condition to that branch
> with comments where I think the assumption is used.

If we cannot easily lift that restriction, that is the right fallback,
yes.

> I've also added a new test for this case.

Thanks.

> > The patch
> > below passes both your test and the already-existing tests in
> > test/src/editfns-tests.el.
> 
> For me after applying your patch, the tests crashed.
> The crash message was hidden in the end of the output:
> 
>    passed  21/24  transpose-nonascii-regions-test-1 (0.000067 sec)
>    passed  22/24  transpose-nonascii-regions-test-2 (0.000068 sec)
>    passed  23/24  transpose-regions-text-properties (0.000074 sec)
> Undo
> Undo
> make[1]: *** [Makefile:181: src/editfns-tests.log] segmentation 
> fault
> make[1]: Leaving directory "/home/gabor/src/build/emacs-29.3/test"
> make: *** [Makefile:247: src/editfns-tests] Error 2

This says the test that crashed was the one of the two new ones you
added, and it is different from the one you presented at the beginning
of this bug report.  So it is a small wonder I didn't see any crash in
my testing.

> With the tests I intended to test all the branches in the code
> where the regions don't touch each other, catching mistakes
> where the wrong length is used.

Can we also have a test where the byte lengths are equal, but the
character lengths aren't?

> I hoped that byte length is not system dependent,

It isn't, not if you consider the internal representation of
characters in Emacs buffers and strings.

> So what differences exist or byte length?

I don't think there are differences, but if you show an example where
you see such a difference, we can discuss the example and see what
happens there.

Last, but not least: with the added tests your patch becomes larger
than what we can accept without your assigning the copyright to the
FSF.  Would you like to start the legal paperwork of copyright
assignment at this time, so that we could accept all of your
contributions without limitations, including any future contributions?
If yes, I will send you the form to fill and instructions to email the
filled form, to start the process.

Thanks.