Patchwork questions on the CVE patches

login
register
mail settings
Submitter Julien Cristau
Date March 30, 2016, 12:39 p.m.
Message ID <20160330123957.GH6200@betterave.cristau.org>
Download mbox | patch
Permalink /patch/14186/
State Not Applicable
Headers show

Comments

Julien Cristau - March 30, 2016, 12:39 p.m.
Hi,

I'm working on backporting yesterday's patches to Debian's supported
releases, and have a couple questions.

First, It looks like https://selenic.com/hg/rev/b732e7f2aba4 is missing
this, since gitopen was removed and gitread has no users left:


Thanks,
Julien
Mateusz Kwapich - March 30, 2016, 7:59 p.m.
Hey,

Thanks for your email.

For the git.py concerns you are right for both. They are not a big deal though
the first is just a dead code - totally harmless. The second is a valid bug -
but it occurs only when a git repo is corrupted. I think those two patches should

be accepted upstream: I’ll resend them to the the mercurial-devel mailing
list one patch per email.

For the mpatch.c let’s wait for Matt to reply: he understands that code much
better than me.

Best,
Mateusz

On 3/30/16, 5:39 AM, "Julien Cristau" <jcristau@debian.org> wrote:

>Hi,

>

>I'm working on backporting yesterday's patches to Debian's supported

>releases, and have a couple questions.

>

>First, It looks like https://selenic.com/hg/rev/b732e7f2aba4 is missing

>this, since gitopen was removed and gitread has no users left:

>

>diff --git a/hgext/convert/git.py b/hgext/convert/git.py

>--- a/hgext/convert/git.py

>+++ b/hgext/convert/git.py

>@@ -48,11 +48,6 @@ class convert_git(converter_source, comm

>     def gitpipe(self, *args, **kwargs):

>         return self._gitcmd(self._run3, *args, **kwargs)

> 

>-    def gitread(self, s):

>-        fh = self.gitopen(s)

>-        data = fh.read()

>-        return data, fh.close()

>-

>     def __init__(self, ui, path, revs=None):

>         super(convert_git, self).__init__(ui, path, revs=revs)

>         commandline.__init__(self, ui, 'git')

>

>Also, https://selenic.com/hg/rev/cdda7b96afff ignores status in the

>'else' branch of getchangedfiles.  Is that intentional, or an oversight?

>

>diff --git a/hgext/convert/git.py b/hgext/convert/git.py

>--- a/hgext/convert/git.py

>+++ b/hgext/convert/git.py

>@@ -350,6 +345,8 @@ class convert_git(converter_source, comm

>             output, status = self.gitrunlines('diff-tree', '--name-only',

>                                               '--root', '-r', version,

>                                               '%s^%s' % (version, i + 1), '--')

>+            if status:

>+                raise error.Abort(_('cannot read changes in %s') % version)

>             changes = [f.rstrip('\n') for f in output]

> 

>         return changes

>

>Finally, my head hurts at backporting

>https://selenic.com/hg/rev/b9714d958e89 past

>https://selenic.com/hg/rev/09e41ac6289d.  Is the following correct?

>Should I give up and just cherry-pick that earlier patch?

>

>--- mercurial-2.2.2.orig/mercurial/mpatch.c

>+++ mercurial-2.2.2/mercurial/mpatch.c

>@@ -212,16 +212,16 @@ static struct flist *decode(const char *

> 

> 	while (data <= end) {

> 		lt->start = getbe32(bin);

> 		lt->end = getbe32(bin + 4);

> 		lt->len = getbe32(bin + 8);

>-		if (lt->start > lt->end)

>-			break; /* sanity check */

> 		bin = data + lt->len;

>+		lt->data = data;

>+		if (lt->start > lt->end || lt->len < 0)

>+			break; /* sanity check */

> 		if (bin < data)

> 			break; /* big data + big (bogus) len can wrap around */

>-		lt->data = data;

> 		data = bin + 12;

> 		lt++;

> 	}

> 

> 	if (bin != end) {

>

>Thanks,

>Julien
Matt Mackall - April 1, 2016, 7:30 p.m.
On Wed, 2016-03-30 at 14:39 +0200, Julien Cristau wrote:
> Hi,
> 
> I'm working on backporting yesterday's patches to Debian's supported
> releases, and have a couple questions.

(lost in my inbox, sorry)

> Finally, my head hurts at backporting
> https://selenic.com/hg/rev/b9714d958e89 past
> https://selenic.com/hg/rev/09e41ac6289d.  Is the following correct?
> Should I give up and just cherry-pick that earlier patch?

Yes, those fixes are also security-sensitive.

-- 
Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/hgext/convert/git.py b/hgext/convert/git.py
--- a/hgext/convert/git.py
+++ b/hgext/convert/git.py
@@ -48,11 +48,6 @@  class convert_git(converter_source, comm
     def gitpipe(self, *args, **kwargs):
         return self._gitcmd(self._run3, *args, **kwargs)
 
-    def gitread(self, s):
-        fh = self.gitopen(s)
-        data = fh.read()
-        return data, fh.close()
-
     def __init__(self, ui, path, revs=None):
         super(convert_git, self).__init__(ui, path, revs=revs)
         commandline.__init__(self, ui, 'git')

Also, https://selenic.com/hg/rev/cdda7b96afff ignores status in the
'else' branch of getchangedfiles.  Is that intentional, or an oversight?

diff --git a/hgext/convert/git.py b/hgext/convert/git.py
--- a/hgext/convert/git.py
+++ b/hgext/convert/git.py
@@ -350,6 +345,8 @@  class convert_git(converter_source, comm
             output, status = self.gitrunlines('diff-tree', '--name-only',
                                               '--root', '-r', version,
                                               '%s^%s' % (version, i + 1), '--')
+            if status:
+                raise error.Abort(_('cannot read changes in %s') % version)
             changes = [f.rstrip('\n') for f in output]
 
         return changes

Finally, my head hurts at backporting
https://selenic.com/hg/rev/b9714d958e89 past
https://selenic.com/hg/rev/09e41ac6289d.  Is the following correct?
Should I give up and just cherry-pick that earlier patch?

--- mercurial-2.2.2.orig/mercurial/mpatch.c
+++ mercurial-2.2.2/mercurial/mpatch.c
@@ -212,16 +212,16 @@  static struct flist *decode(const char *
 
 	while (data <= end) {
 		lt->start = getbe32(bin);
 		lt->end = getbe32(bin + 4);
 		lt->len = getbe32(bin + 8);
-		if (lt->start > lt->end)
-			break; /* sanity check */
 		bin = data + lt->len;
+		lt->data = data;
+		if (lt->start > lt->end || lt->len < 0)
+			break; /* sanity check */
 		if (bin < data)
 			break; /* big data + big (bogus) len can wrap around */
-		lt->data = data;
 		data = bin + 12;
 		lt++;
 	}
 
 	if (bin != end) {