Patchwork [stable] cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())

login
register
mail settings
Submitter Manuel Jacob
Date May 14, 2020, 12:10 a.m.
Message ID <fdbe20620a267f2cc8fb.1589415049@tmp>
Download mbox | patch
Permalink /patch/46309/
State New
Headers show

Comments

Manuel Jacob - May 14, 2020, 12:10 a.m.
# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1589415041 -7200
#      Thu May 14 02:10:41 2020 +0200
# Branch stable
# Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
# Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())

On old Python versions (prior to 2.7.4), old-style 'buffer' objects were not
recognized by the new-style buffer API. See https://bugs.python.org/issue10211
for details.

Since old-style buffers are deprecated on Python 3 and the Python version that
fixes this issue was released over 7 years ago, I'm not actually proposing
that this patch should be committed. It might still be helpful for people
compiling Mercurial on very old versions of Python.
Gregory Szorc - May 14, 2020, 7:15 p.m.
On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> wrote:

> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1589415041 -7200
> #      Thu May 14 02:10:41 2020 +0200
> # Branch stable
> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
> cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())
>

I'm -0 on committing this because nobody should be running Python <2.7.4 in
2020. Although apparently you are. May I ask how you managed to discover
this? (The original patch landed in October 2018 and as far as I know
nobody has reported an issue until now.)


>
> On old Python versions (prior to 2.7.4), old-style 'buffer' objects were
> not
> recognized by the new-style buffer API. See
> https://bugs.python.org/issue10211
> for details.
>
> Since old-style buffers are deprecated on Python 3 and the Python version
> that
> fixes this issue was released over 7 years ago, I'm not actually proposing
> that this patch should be committed. It might still be helpful for people
> compiling Mercurial on very old versions of Python.
>
> diff --git a/mercurial/cext/mpatch.c b/mercurial/cext/mpatch.c
> --- a/mercurial/cext/mpatch.c
> +++ b/mercurial/cext/mpatch.c
> @@ -50,25 +50,24 @@
>
>  struct mpatch_flist *cpygetitem(void *bins, ssize_t pos)
>  {
> -       Py_buffer buffer;
> -       struct mpatch_flist *res = NULL;
> +       const char *buffer;
> +       struct mpatch_flist *res;
> +       ssize_t blen;
>         int r;
>
>         PyObject *tmp = PyList_GetItem((PyObject *)bins, pos);
>         if (!tmp) {
>                 return NULL;
>         }
> -       if (PyObject_GetBuffer(tmp, &buffer, PyBUF_CONTIG_RO)) {
> +       if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t *)&blen)) {
>                 return NULL;
>         }
> -       if ((r = mpatch_decode(buffer.buf, buffer.len, &res)) < 0) {
> +       if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
>                 if (!PyErr_Occurred()) {
>                         setpyerr(r);
>                 }
> -               res = NULL;
> +               return NULL;
>         }
> -
> -       PyBuffer_Release(&buffer);
>         return res;
>  }
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - May 14, 2020, 7:17 p.m.
On Thu, May 14, 2020 at 12:15 PM Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> wrote:
>
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1589415041 -7200
>> #      Thu May 14 02:10:41 2020 +0200
>> # Branch stable
>> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
>> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
>> cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())
>>
>
> I'm -0 on committing this because nobody should be running Python <2.7.4
> in 2020. Although apparently you are. May I ask how you managed to discover
> this? (The original patch landed in October 2018 and as far as I know
> nobody has reported an issue until now.)
>

I'll also mention that python-zstandard uses Py_buffer heavily. I'm very
curious if `hg clone` works at all against a server using zstandard or with
zstandard revlog compression. I wouldn't at all be surprised if something
broke somewhere.


>
>
>>
>> On old Python versions (prior to 2.7.4), old-style 'buffer' objects were
>> not
>> recognized by the new-style buffer API. See
>> https://bugs.python.org/issue10211
>> for details.
>>
>> Since old-style buffers are deprecated on Python 3 and the Python version
>> that
>> fixes this issue was released over 7 years ago, I'm not actually proposing
>> that this patch should be committed. It might still be helpful for people
>> compiling Mercurial on very old versions of Python.
>>
>> diff --git a/mercurial/cext/mpatch.c b/mercurial/cext/mpatch.c
>> --- a/mercurial/cext/mpatch.c
>> +++ b/mercurial/cext/mpatch.c
>> @@ -50,25 +50,24 @@
>>
>>  struct mpatch_flist *cpygetitem(void *bins, ssize_t pos)
>>  {
>> -       Py_buffer buffer;
>> -       struct mpatch_flist *res = NULL;
>> +       const char *buffer;
>> +       struct mpatch_flist *res;
>> +       ssize_t blen;
>>         int r;
>>
>>         PyObject *tmp = PyList_GetItem((PyObject *)bins, pos);
>>         if (!tmp) {
>>                 return NULL;
>>         }
>> -       if (PyObject_GetBuffer(tmp, &buffer, PyBUF_CONTIG_RO)) {
>> +       if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t *)&blen)) {
>>                 return NULL;
>>         }
>> -       if ((r = mpatch_decode(buffer.buf, buffer.len, &res)) < 0) {
>> +       if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
>>                 if (!PyErr_Occurred()) {
>>                         setpyerr(r);
>>                 }
>> -               res = NULL;
>> +               return NULL;
>>         }
>> -
>> -       PyBuffer_Release(&buffer);
>>         return res;
>>  }
>>
>>
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
Augie Fackler - May 14, 2020, 9:28 p.m.
> On May 14, 2020, at 15:15, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> 
> On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1589415041 -7200
>> #      Thu May 14 02:10:41 2020 +0200
>> # Branch stable
>> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
>> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
>> cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())
>> 
> I'm -0 on committing this because nobody should be running Python <2.7.4 in 2020. Although apparently you are. May I ask how you managed to discover this? (The original patch landed in October 2018 and as far as I know nobody has reported an issue until now.)

Yeah, a regression from October 2018 that matters for Pythons released more than seven years ago isn't going to make the cut.

In what environment are you encountering this issue Manuel?
Manuel Jacob - May 14, 2020, 10:08 p.m.
On 2020-05-14 21:17, Gregory Szorc wrote:
> On Thu, May 14, 2020 at 12:15 PM Gregory Szorc 
> <gregory.szorc@gmail.com>
> wrote:
> 
>> On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> 
>> wrote:
>> 
>>> # HG changeset patch
>>> # User Manuel Jacob <me@manueljacob.de>
>>> # Date 1589415041 -7200
>>> #      Thu May 14 02:10:41 2020 +0200
>>> # Branch stable
>>> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
>>> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
>>> cext: back out ec3c06a1c554 (use modern buffer protocol in 
>>> mpatch_flist())
>>> 
>> 
>> I'm -0 on committing this because nobody should be running Python 
>> <2.7.4
>> in 2020. Although apparently you are. May I ask how you managed to 
>> discover
>> this? (The original patch landed in October 2018 and as far as I know
>> nobody has reported an issue until now.)

Someone asked on the Mercurial (non-devel) list why Mercurial was 
failing on his machine (running Python 2.7.3). I prepared the patch 
specifically to work around this issue. That's why I suggested myself in 
the patch description that it should probably not be committed. The user 
who reported the problem could work around the problem by disabling the 
C extension.

> I'll also mention that python-zstandard uses Py_buffer heavily. I'm 
> very
> curious if `hg clone` works at all against a server using zstandard or 
> with
> zstandard revlog compression. I wouldn't at all be surprised if 
> something
> broke somewhere.

Sounds like it would be a good idea to raise an error for too old Python 
versions. This was also proposed by Marcin Kasperski on the other 
mailing list.

What would be a good minimum Python version?

It should of course be at least Python 2.7.4, to prevent the obscure 
crash from happening.

Some security features of Python 3.x were backported to Python 2.7.9 
(see PEP 466). When requiring at least Python 2.7.9, we can make raise 
the minimum security standard of Mercurial (quote from 
mercurial.sslutil: "Depending on the version of Python being used, 
SSL/TLS support is either modern/secure or legacy/insecure. Many 
operations in this module have separate code paths depending on support 
in Python.").

The older the Python, the higher the chance of running into bugs. But 
unless there's a specific reason, I tend to say that we should not 
forbid old Python versions just because we can (here I'm taking the 
perspective of others; for me personally, it would be fine to drop 
Python 2.7 support completely).

My proposal:

* Change the stable branch to raise an error if the Python version is 
older than Python 2.7.4.
* Change the default branch to raise an error if the Python version is 
older than Python 2.7.9.
Pierre-Yves David - May 14, 2020, 10:25 p.m.
On 5/14/20 11:28 PM, Augie Fackler wrote:
> 
> 
>> On May 14, 2020, at 15:15, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>>
>> On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> wrote:
>>> # HG changeset patch
>>> # User Manuel Jacob <me@manueljacob.de>
>>> # Date 1589415041 -7200
>>> #      Thu May 14 02:10:41 2020 +0200
>>> # Branch stable
>>> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
>>> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
>>> cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())
>>>
>> I'm -0 on committing this because nobody should be running Python <2.7.4 in 2020. Although apparently you are. May I ask how you managed to discover this? (The original patch landed in October 2018 and as far as I know nobody has reported an issue until now.)
> 
> Yeah, a regression from October 2018 that matters for Pythons released more than seven years ago isn't going to make the cut.
> 
> In what environment are you encountering this issue Manuel?

The context seems to be 
https://www.mercurial-scm.org/pipermail/mercurial/2020-May/051937.html
Augie Fackler - May 14, 2020, 10:27 p.m.
> On May 14, 2020, at 6:25 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 5/14/20 11:28 PM, Augie Fackler wrote:
>>> On May 14, 2020, at 15:15, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>>> 
>>> On Wed, May 13, 2020 at 5:15 PM Manuel Jacob <me@manueljacob.de> wrote:
>>>> # HG changeset patch
>>>> # User Manuel Jacob <me@manueljacob.de>
>>>> # Date 1589415041 -7200
>>>> #      Thu May 14 02:10:41 2020 +0200
>>>> # Branch stable
>>>> # Node ID fdbe20620a267f2cc8fb01b60a293117f00cebdb
>>>> # Parent  cf3e07d7648a4371ce584d15dd692e7a6845792f
>>>> cext: back out ec3c06a1c554 (use modern buffer protocol in mpatch_flist())
>>>> 
>>> I'm -0 on committing this because nobody should be running Python <2.7.4 in 2020. Although apparently you are. May I ask how you managed to discover this? (The original patch landed in October 2018 and as far as I know nobody has reported an issue until now.)
>> Yeah, a regression from October 2018 that matters for Pythons released more than seven years ago isn't going to make the cut.
>> In what environment are you encountering this issue Manuel?
> 
> The context seems to be https://www.mercurial-scm.org/pipermail/mercurial/2020-May/051937.html


Ah.

> Wheezy also benefits from Long Term Support (LTS) until the end of May 2018.

Welp. I think the user should probably see about migrating. I think it’s more likely we drop 2.7 entirely than worry about 2.7.3 and earlier at this point, sorry!

Patch

diff --git a/mercurial/cext/mpatch.c b/mercurial/cext/mpatch.c
--- a/mercurial/cext/mpatch.c
+++ b/mercurial/cext/mpatch.c
@@ -50,25 +50,24 @@ 
 
 struct mpatch_flist *cpygetitem(void *bins, ssize_t pos)
 {
-	Py_buffer buffer;
-	struct mpatch_flist *res = NULL;
+	const char *buffer;
+	struct mpatch_flist *res;
+	ssize_t blen;
 	int r;
 
 	PyObject *tmp = PyList_GetItem((PyObject *)bins, pos);
 	if (!tmp) {
 		return NULL;
 	}
-	if (PyObject_GetBuffer(tmp, &buffer, PyBUF_CONTIG_RO)) {
+	if (PyObject_AsCharBuffer(tmp, &buffer, (Py_ssize_t *)&blen)) {
 		return NULL;
 	}
-	if ((r = mpatch_decode(buffer.buf, buffer.len, &res)) < 0) {
+	if ((r = mpatch_decode(buffer, blen, &res)) < 0) {
 		if (!PyErr_Occurred()) {
 			setpyerr(r);
 		}
-		res = NULL;
+		return NULL;
 	}
-
-	PyBuffer_Release(&buffer);
 	return res;
 }