Patchwork [1,of,7] parsers: fix leak of err when asciilower hits a unicode decode error

login
register
mail settings
Submitter Augie Fackler
Date Jan. 23, 2015, 9:06 p.m.
Message ID <3435cf143b791612214a.1422047198@arthedain.pit.corp.google.com>
Download mbox | patch
Permalink /patch/7539/
State Accepted
Commit fefa5f2a1730acac2300e1af756cc913270eb0ca
Headers show

Comments

Augie Fackler - Jan. 23, 2015, 9:06 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1422044344 18000
#      Fri Jan 23 15:19:04 2015 -0500
# Branch stable
# Node ID 3435cf143b791612214a871337e8549e7d5c26a6
# Parent  de519517f597abd733961a8c549074eecb6c0ab3
parsers: fix leak of err when asciilower hits a unicode decode error

This is one of many errors detected in parsers.c by cpychecker[1]. I
haven't gone through all of them yet.

1: https://gcc-python-plugin.readthedocs.org/en/latest/index.html
Martin von Zweigbergk - Jan. 23, 2015, 9:21 p.m.
Nice finds!

On Fri Jan 23 2015 at 1:08:40 PM Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422044344 18000
> #      Fri Jan 23 15:19:04 2015 -0500
> # Branch stable
> # Node ID 3435cf143b791612214a871337e8549e7d5c26a6
> # Parent  de519517f597abd733961a8c549074eecb6c0ab3
> parsers: fix leak of err when asciilower hits a unicode decode error
>
> This is one of many errors detected in parsers.c by cpychecker[1]. I
> haven't gone through all of them yet.
>
> 1: https://gcc-python-plugin.readthedocs.org/en/latest/index.html
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -45,11 +45,11 @@ static char lowertable[128] = {
>         '\x30', '\x31', '\x32', '\x33', '\x34', '\x35', '\x36', '\x37',
>         '\x38', '\x39', '\x3a', '\x3b', '\x3c', '\x3d', '\x3e', '\x3f',
>         '\x40',
> -               '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /*
> A-G */
> +               '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /*
> A-G */
>         '\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f', /*
> H-O */
>         '\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77', /*
> P-W */
>         '\x78', '\x79', '\x7a',                                         /*
> X-Z */
> -                               '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
> +                               '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
>

Were these changes intentional? (Replacing spaces by tabs? I'm not sure if
it's just Inbox that presents tabs as spaces. Too lazy to check.)


>         '\x60', '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67',
>         '\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f',
>         '\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77',
> @@ -115,6 +115,7 @@ static PyObject *asciilower(PyObject *se
>                                 "ascii", str, len, i, (i + 1),
>                                 "unexpected code byte");
>                         PyErr_SetObject(PyExc_UnicodeDecodeError, err);
> +                       Py_XDECREF(err);
>

Could be Py_DECREF instead? It seems like err should never be NULL.


>                         goto quit;
>                 }
>                 newstr[i] = lowertable[(unsigned char)c];
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - Jan. 23, 2015, 9:53 p.m.
On Jan 23, 2015, at 4:21 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:

> Nice finds!
> 
> On Fri Jan 23 2015 at 1:08:40 PM Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422044344 18000
> #      Fri Jan 23 15:19:04 2015 -0500
> # Branch stable
> # Node ID 3435cf143b791612214a871337e8549e7d5c26a6
> # Parent  de519517f597abd733961a8c549074eecb6c0ab3
> parsers: fix leak of err when asciilower hits a unicode decode error
> 
> This is one of many errors detected in parsers.c by cpychecker[1]. I
> haven't gone through all of them yet.
> 
> 1: https://gcc-python-plugin.readthedocs.org/en/latest/index.html
> 
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -45,11 +45,11 @@ static char lowertable[128] = {
>         '\x30', '\x31', '\x32', '\x33', '\x34', '\x35', '\x36', '\x37',
>         '\x38', '\x39', '\x3a', '\x3b', '\x3c', '\x3d', '\x3e', '\x3f',
>         '\x40',
> -               '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */
> +               '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */
>         '\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f', /* H-O */
>         '\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77', /* P-W */
>         '\x78', '\x79', '\x7a',                                         /* X-Z */
> -                               '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
> +                               '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
> 
> Were these changes intentional? (Replacing spaces by tabs? I'm not sure if it's just Inbox that presents tabs as spaces. Too lazy to check.)

No, that was emacs being jerk and "fixing" it for me. I can do a resend, or just drop this hunk.

>  
>         '\x60', '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67',
>         '\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f',
>         '\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77',
> @@ -115,6 +115,7 @@ static PyObject *asciilower(PyObject *se
>                                 "ascii", str, len, i, (i + 1),
>                                 "unexpected code byte");
>                         PyErr_SetObject(PyExc_UnicodeDecodeError, err);
> +                       Py_XDECREF(err);
> 
> Could be Py_DECREF instead? It seems like err should never be NULL.
>  
>                         goto quit;
>                 }
>                 newstr[i] = lowertable[(unsigned char)c];
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Sean Farley - Jan. 23, 2015, 10:06 p.m.
Augie Fackler writes:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1422044344 18000
> #      Fri Jan 23 15:19:04 2015 -0500
> # Branch stable
> # Node ID 3435cf143b791612214a871337e8549e7d5c26a6
> # Parent  de519517f597abd733961a8c549074eecb6c0ab3
> parsers: fix leak of err when asciilower hits a unicode decode error
>
> This is one of many errors detected in parsers.c by cpychecker[1]. I
> haven't gone through all of them yet.
>
> 1: https://gcc-python-plugin.readthedocs.org/en/latest/index.html
>
> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
> --- a/mercurial/parsers.c
> +++ b/mercurial/parsers.c
> @@ -45,11 +45,11 @@ static char lowertable[128] = {
>  	'\x30', '\x31', '\x32', '\x33', '\x34', '\x35', '\x36', '\x37',
>  	'\x38', '\x39', '\x3a', '\x3b', '\x3c', '\x3d', '\x3e', '\x3f',
>  	'\x40',
> -	        '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */
> +		'\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */

What kind of whitespace mode are you using that reindents these bits
incorrectly?

>  	'\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f', /* H-O */
>  	'\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77', /* P-W */
>  	'\x78', '\x79', '\x7a',                                         /* X-Z */
> -	                        '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
> +				'\x5b', '\x5c', '\x5d', '\x5e', '\x5f',

Same question here.

Patch

diff --git a/mercurial/parsers.c b/mercurial/parsers.c
--- a/mercurial/parsers.c
+++ b/mercurial/parsers.c
@@ -45,11 +45,11 @@  static char lowertable[128] = {
 	'\x30', '\x31', '\x32', '\x33', '\x34', '\x35', '\x36', '\x37',
 	'\x38', '\x39', '\x3a', '\x3b', '\x3c', '\x3d', '\x3e', '\x3f',
 	'\x40',
-	        '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */
+		'\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67', /* A-G */
 	'\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f', /* H-O */
 	'\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77', /* P-W */
 	'\x78', '\x79', '\x7a',                                         /* X-Z */
-	                        '\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
+				'\x5b', '\x5c', '\x5d', '\x5e', '\x5f',
 	'\x60', '\x61', '\x62', '\x63', '\x64', '\x65', '\x66', '\x67',
 	'\x68', '\x69', '\x6a', '\x6b', '\x6c', '\x6d', '\x6e', '\x6f',
 	'\x70', '\x71', '\x72', '\x73', '\x74', '\x75', '\x76', '\x77',
@@ -115,6 +115,7 @@  static PyObject *asciilower(PyObject *se
 				"ascii", str, len, i, (i + 1),
 				"unexpected code byte");
 			PyErr_SetObject(PyExc_UnicodeDecodeError, err);
+			Py_XDECREF(err);
 			goto quit;
 		}
 		newstr[i] = lowertable[(unsigned char)c];