GNU bug report logs - #20490
[PATCH] fixup: reference to uninitialized variable with invalid sequence

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20490 in the body.
You can then email your comments to 20490 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: bug-sed <at> gnu.org
Subject: Re: [PATCH] fixup: reference to uninitialized variable with invalid
 sequence
Date: Sat, 2 May 2015 19:09:16 -0700

[Message part 1 (text/plain, inline)]

On Wed, Nov 5, 2014 at 7:36 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> Uninitialized variable are referred with invalid sequence in
> str_append_modified().
>
> When mbrtowc() returns (size_t) -1, wc is not changed, even if wc is
> uninitialized.  below may return unexpected result in order that the
> value is referred at a following position in source code.
>
>   $ echo a | LC_ALL=ja_JP.eucJP ./sed/sed -e 's/a/b\U\xb2c/'

Thank you for the patch and reproducer.
I've made some small improvements to the actual patch and
wrote a valgrind-using test that I'm adding to the test suite.
I've included your patch with an adjusted log, followed by
the changes I made to it in a separate commit. That commit
is separate solely to show what I've done; I will squash into your
commit before I push, followed by the test-adding commit.

[0001-sed-avoid-a-UMR-bug-when-processing-an-invalid-multi.patch (application/octet-stream, attachment)]

[0002-fixup-for-bug-fix-to-be-squashed.patch (application/octet-stream, attachment)]

[0003-tests-test-for-just-fixed-UMR-bug.patch (application/octet-stream, attachment)]

Message #10 received at 20490-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 20490-done <at> debbugs.gnu.org
Subject: Re: [PATCH] fixup: reference to uninitialized variable with invalid
 sequence
Date: Sun, 3 May 2015 10:06:00 -0700

On Sat, May 2, 2015 at 7:09 PM, Jim Meyering <jim <at> meyering.net> wrote:
> On Wed, Nov 5, 2014 at 7:36 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>> Uninitialized variable are referred with invalid sequence in
>> str_append_modified().
>>
>> When mbrtowc() returns (size_t) -1, wc is not changed, even if wc is
>> uninitialized.  below may return unexpected result in order that the
>> value is referred at a following position in source code.
>>
>>   $ echo a | LC_ALL=ja_JP.eucJP ./sed/sed -e 's/a/b\U\xb2c/'
>
> Thank you for the patch and reproducer.
> I've made some small improvements to the actual patch and
> wrote a valgrind-using test that I'm adding to the test suite.
> I've included your patch with an adjusted log, followed by
> the changes I made to it in a separate commit. That commit
> is separate solely to show what I've done; I will squash into your
> commit before I push, followed by the test-adding commit.

I've updated the commit log to reference this just-closed issue,
with this line:

  This addresses http://debbugs.gnu.org/20490

Then pushed.

Message #13 received at 20490 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: 20490 <at> debbugs.gnu.org,
 jim <at> meyering.net,
 jim <at> meyering.net
Subject: Re: bug#20490: [PATCH] fixup: reference to uninitialized variable
 with invalid sequence
Date: Tue, 05 May 2015 18:41:52 +0900

On Sun, 3 May 2015 10:06:00 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> > Thank you for the patch and reproducer.
> > I've made some small improvements to the actual patch and
> > wrote a valgrind-using test that I'm adding to the test suite.
> > I've included your patch with an adjusted log, followed by
> > the changes I made to it in a separate commit. That commit
> > is separate solely to show what I've done; I will squash into your
> > commit before I push, followed by the test-adding commit.
> 
> I've updated the commit log to reference this just-closed issue,
> with this line:
> 
>   This addresses http://debbugs.gnu.org/20490
> 
> Then pushed.

Thanks for review and ajustment.  I confirmed them.

Message #16 received at 20490 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: jim <at> meyering.net
Cc: 20490 <at> debbugs.gnu.org
Subject: Re: bug#20490: [PATCH] fixup: reference to uninitialized variable
 with invalid sequence
Date: Wed, 06 May 2015 08:58:47 +0900

[Message part 1 (text/plain, inline)]

On Sun, 3 May 2015 10:06:00 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> On Sat, May 2, 2015 at 7:09 PM, Jim Meyering <jim <at> meyering.net> wrote:
> > On Wed, Nov 5, 2014 at 7:36 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> >> Uninitialized variable are referred with invalid sequence in
> >> str_append_modified().
> >>
> >> When mbrtowc() returns (size_t) -1, wc is not changed, even if wc is
> >> uninitialized.  below may return unexpected result in order that the
> >> value is referred at a following position in source code.
> >>
> >>   $ echo a | LC_ALL=ja_JP.eucJP ./sed/sed -e 's/a/b\U\xb2c/'
> >
> > Thank you for the patch and reproducer.
> > I've made some small improvements to the actual patch and
> > wrote a valgrind-using test that I'm adding to the test suite.
> > I've included your patch with an adjusted log, followed by
> > the changes I made to it in a separate commit. That commit
> > is separate solely to show what I've done; I will squash into your
> > commit before I push, followed by the test-adding commit.
> 
> I've updated the commit log to reference this just-closed issue,
> with this line:
> 
>   This addresses http://debbugs.gnu.org/20490
> 
> Then pushed.

I see that this bug itself is correctly fixed by the patch.

I ran new test on CentOS 5.10 (x86), and ran accross an error in
attachment even after applying the patch.

[test-suite.log (text/plain, attachment)]

Message #19 received at 20490 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 20490 <at> debbugs.gnu.org
Subject: Re: bug#20490: [PATCH] fixup: reference to uninitialized variable
 with invalid sequence
Date: Wed, 6 May 2015 19:18:01 -0700

[Message part 1 (text/plain, inline)]

On Tue, May 5, 2015 at 4:58 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>
> On Sun, 3 May 2015 10:06:00 -0700
> Jim Meyering <jim <at> meyering.net> wrote:
>
>> On Sat, May 2, 2015 at 7:09 PM, Jim Meyering <jim <at> meyering.net> wrote:
>> > On Wed, Nov 5, 2014 at 7:36 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
>> >> Uninitialized variable are referred with invalid sequence in
>> >> str_append_modified().
>> >>
>> >> When mbrtowc() returns (size_t) -1, wc is not changed, even if wc is
>> >> uninitialized.  below may return unexpected result in order that the
>> >> value is referred at a following position in source code.
>> >>
>> >>   $ echo a | LC_ALL=ja_JP.eucJP ./sed/sed -e 's/a/b\U\xb2c/'
>> >
>> > Thank you for the patch and reproducer.
>> > I've made some small improvements to the actual patch and
>> > wrote a valgrind-using test that I'm adding to the test suite.
>> > I've included your patch with an adjusted log, followed by
>> > the changes I made to it in a separate commit. That commit
>> > is separate solely to show what I've done; I will squash into your
>> > commit before I push, followed by the test-adding commit.
>>
>> I've updated the commit log to reference this just-closed issue,
>> with this line:
>>
>>   This addresses http://debbugs.gnu.org/20490
>>
>> Then pushed.
>
> I see that this bug itself is correctly fixed by the patch.
>
> I ran new test on CentOS 5.10 (x86), and ran accross an error in
> attachment even after applying the patch.

Thank you for the report and for testing.
That is seems to be due to a bug in that old version of valgrind:

  +valgrind: m_debuginfo/readdwarf.c:2262 (copy_convert_CfiExpr_tree):
Assertion 'srcix >= 0 && srcix < VG_(sizeXA)(srcxa)' failed.

that is not exposed when the require_valgrind_
shell function tests valgrind against "true".

We can easily accommodate that, so I wrote the attached patch.
Can you verify that it causes the test to be skipped on your
system?

[0001-tests-skip-the-new-test-in-presence-of-buggy-valgrin.patch (application/octet-stream, attachment)]

Message #22 received at 20490 <at> debbugs.gnu.org (full text, mbox):

From: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
To: Jim Meyering <jim <at> meyering.net>
Cc: 20490 <at> debbugs.gnu.org
Subject: Re: bug#20490: [PATCH] fixup: reference to uninitialized variable
 with invalid sequence
Date: Thu, 07 May 2015 22:53:51 +0900

On Wed, 6 May 2015 19:18:01 -0700
Jim Meyering <jim <at> meyering.net> wrote:

> On Tue, May 5, 2015 at 4:58 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> >
> > On Sun, 3 May 2015 10:06:00 -0700
> > Jim Meyering <jim <at> meyering.net> wrote:
> >
> >> On Sat, May 2, 2015 at 7:09 PM, Jim Meyering <jim <at> meyering.net> wrote:
> >> > On Wed, Nov 5, 2014 at 7:36 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> >> >> Uninitialized variable are referred with invalid sequence in
> >> >> str_append_modified().
> >> >>
> >> >> When mbrtowc() returns (size_t) -1, wc is not changed, even if wc is
> >> >> uninitialized.  below may return unexpected result in order that the
> >> >> value is referred at a following position in source code.
> >> >>
> >> >>   $ echo a | LC_ALL=ja_JP.eucJP ./sed/sed -e 's/a/b\U\xb2c/'
> >> >
> >> > Thank you for the patch and reproducer.
> >> > I've made some small improvements to the actual patch and
> >> > wrote a valgrind-using test that I'm adding to the test suite.
> >> > I've included your patch with an adjusted log, followed by
> >> > the changes I made to it in a separate commit. That commit
> >> > is separate solely to show what I've done; I will squash into your
> >> > commit before I push, followed by the test-adding commit.
> >>
> >> I've updated the commit log to reference this just-closed issue,
> >> with this line:
> >>
> >>   This addresses http://debbugs.gnu.org/20490
> >>
> >> Then pushed.
> >
> > I see that this bug itself is correctly fixed by the patch.
> >
> > I ran new test on CentOS 5.10 (x86), and ran accross an error in
> > attachment even after applying the patch.
> 
> Thank you for the report and for testing.
> That is seems to be due to a bug in that old version of valgrind:
> 
>   +valgrind: m_debuginfo/readdwarf.c:2262 (copy_convert_CfiExpr_tree):
> Assertion 'srcix >= 0 && srcix < VG_(sizeXA)(srcxa)' failed.
> 
> that is not exposed when the require_valgrind_
> shell function tests valgrind against "true".
> 
> We can easily accommodate that, so I wrote the attached patch.
> Can you verify that it causes the test to be skipped on your
> system?

Thanks for fixing the bug.  I re-try with new version of valgrind, and
succeed for the test.  I also confirmed that the test is skipped with
old version.

......
PASS: help-version.sh
invalid-mb-seq-UMR.sh: skipped test: requires a working valgrind
SKIP: invalid-mb-seq-UMR.sh
PASS: range-overlap.sh
============================================================================
Testsuite summary for GNU sed 4.2.2.81-aeb3-dirty
============================================================================
# TOTAL: 69
# PASS:  68
# SKIP:  1
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================
......

Message #25 received at 20490 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Norihiro Tanaka <noritnk <at> kcn.ne.jp>
Cc: 20490 <20490 <at> debbugs.gnu.org>
Subject: Re: bug#20490: [PATCH] fixup: reference to uninitialized variable
 with invalid sequence
Date: Thu, 7 May 2015 08:20:02 -0700

On Thu, May 7, 2015 at 6:53 AM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
> On Wed, 6 May 2015 19:18:01 -0700
> Jim Meyering <jim <at> meyering.net> wrote:
>
>> On Tue, May 5, 2015 at 4:58 PM, Norihiro Tanaka <noritnk <at> kcn.ne.jp> wrote:
...
>> We can easily accommodate that, so I wrote the attached patch.
>> Can you verify that it causes the test to be skipped on your
>> system?
>
> Thanks for fixing the bug.  I re-try with new version of valgrind, and
> succeed for the test.  I also confirmed that the test is skipped with
> old version.

Thanks.
Pushed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.