Patchwork [1,of,5,STABLE] base85: fix leak on error return from b85decode()

login
register
mail settings
Submitter Yuya Nishihara
Date Sept. 5, 2018, 1:58 p.m.
Message ID <33c58a7ff5d54660c5c1.1536155903@mimosa>
Download mbox | patch
Permalink /patch/34336/
State Accepted
Headers show

Comments

Yuya Nishihara - Sept. 5, 2018, 1:58 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1536148658 -32400
#      Wed Sep 05 20:57:38 2018 +0900
# Branch stable
# Node ID 33c58a7ff5d54660c5c15a007d8297119cf9ab82
# Parent  e574cae381b66a8543c8183a76768abcb55d98fc
base85: fix leak on error return from b85decode()

Spotted by ASAN.

We don't need to initialize 'out' to NULL, but I decided to do that for
clarity.
via Mercurial-devel - Sept. 5, 2018, 3:20 p.m.
On Wed, Sep 5, 2018 at 7:00 AM Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1536148658 -32400
> #      Wed Sep 05 20:57:38 2018 +0900
> # Branch stable
> # Node ID 33c58a7ff5d54660c5c15a007d8297119cf9ab82
> # Parent  e574cae381b66a8543c8183a76768abcb55d98fc
> base85: fix leak on error return from b85decode()
>
> Spotted by ASAN.
>
> We don't need to initialize 'out' to NULL, but I decided to do that for
> clarity.
>

Looks like that would lead to a "declaration after statement" warning or
whatever it's called. I might drop it in flight if it does indeed cause
that.


>
> diff --git a/mercurial/cext/base85.c b/mercurial/cext/base85.c
> --- a/mercurial/cext/base85.c
> +++ b/mercurial/cext/base85.c
> @@ -77,7 +77,7 @@ static PyObject *b85encode(PyObject *sel
>
>  static PyObject *b85decode(PyObject *self, PyObject *args)
>  {
> -       PyObject *out;
> +       PyObject *out = NULL;
>         const char *text;
>         char *dst;
>         Py_ssize_t len, i, j, olen, cap;
>
>
via Mercurial-devel - Sept. 5, 2018, 3:38 p.m.
On Wed, Sep 5, 2018 at 8:20 AM Martin von Zweigbergk <martinvonz@google.com>
wrote:

>
>
> On Wed, Sep 5, 2018 at 7:00 AM Yuya Nishihara <yuya@tcha.org> wrote:
>
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1536148658 -32400
>> #      Wed Sep 05 20:57:38 2018 +0900
>> # Branch stable
>> # Node ID 33c58a7ff5d54660c5c15a007d8297119cf9ab82
>> # Parent  e574cae381b66a8543c8183a76768abcb55d98fc
>> base85: fix leak on error return from b85decode()
>>
>> Spotted by ASAN.
>>
>> We don't need to initialize 'out' to NULL, but I decided to do that for
>> clarity.
>>
>
> Looks like that would lead to a "declaration after statement" warning or
> whatever it's called. I might drop it in flight if it does indeed cause
> that.
>

I was wrong, it's still a declaration even if it contains an initialization.


>
>>
>> diff --git a/mercurial/cext/base85.c b/mercurial/cext/base85.c
>> --- a/mercurial/cext/base85.c
>> +++ b/mercurial/cext/base85.c
>> @@ -77,7 +77,7 @@ static PyObject *b85encode(PyObject *sel
>>
>>  static PyObject *b85decode(PyObject *self, PyObject *args)
>>  {
>> -       PyObject *out;
>> +       PyObject *out = NULL;
>>         const char *text;
>>         char *dst;
>>         Py_ssize_t len, i, j, olen, cap;
>>
>>
>

Patch

diff --git a/mercurial/cext/base85.c b/mercurial/cext/base85.c
--- a/mercurial/cext/base85.c
+++ b/mercurial/cext/base85.c
@@ -77,7 +77,7 @@  static PyObject *b85encode(PyObject *sel
 
 static PyObject *b85decode(PyObject *self, PyObject *args)
 {
-	PyObject *out;
+	PyObject *out = NULL;
 	const char *text;
 	char *dst;
 	Py_ssize_t len, i, j, olen, cap;
@@ -104,27 +104,33 @@  static PyObject *b85decode(PyObject *sel
 			cap = 4;
 		for (j = 0; j < cap; i++, j++) {
 			c = b85dec[(int)*text++] - 1;
-			if (c < 0)
-				return PyErr_Format(
+			if (c < 0) {
+				PyErr_Format(
 				    PyExc_ValueError,
 				    "bad base85 character at position %d",
 				    (int)i);
+				goto bail;
+			}
 			acc = acc * 85 + c;
 		}
 		if (i++ < len) {
 			c = b85dec[(int)*text++] - 1;
-			if (c < 0)
-				return PyErr_Format(
+			if (c < 0) {
+				PyErr_Format(
 				    PyExc_ValueError,
 				    "bad base85 character at position %d",
 				    (int)i);
+				goto bail;
+			}
 			/* overflow detection: 0xffffffff == "|NsC0",
 			 * "|NsC" == 0x03030303 */
-			if (acc > 0x03030303 || (acc *= 85) > 0xffffffff - c)
-				return PyErr_Format(
+			if (acc > 0x03030303 || (acc *= 85) > 0xffffffff - c) {
+				PyErr_Format(
 				    PyExc_ValueError,
 				    "bad base85 sequence at position %d",
 				    (int)i);
+				goto bail;
+			}
 			acc += c;
 		}
 
@@ -141,6 +147,9 @@  static PyObject *b85decode(PyObject *sel
 	}
 
 	return out;
+bail:
+	Py_XDECREF(out);
+	return NULL;
 }
 
 static char base85_doc[] = "Base85 Data Encoding";