Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

valgrind errors in t/op/vec.t #13638

Closed
p5pRT opened this issue Mar 3, 2014 · 9 comments
Closed

valgrind errors in t/op/vec.t #13638

p5pRT opened this issue Mar 3, 2014 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Mar 3, 2014

Migrated from rt.perl.org#121366 (status was 'resolved')

Searchable as RT121366$

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2014

From @khwilliamson

A bisect yields this​:

032061d is the first bad commit
commit 032061d
Author​: Brian Fraser <fraserbn@​gmail.com>
Date​: Sat May 26 17​:35​:26 2012 -0300

  Fix for [perl #9423] vec assignments generate 2 warnings
 
  Before this commit, this​:
 
  vec(my $v,0,1) = 1;
 
  would've produced four warnings about uninitialized values;
  however, the ticket argued that these were spurious.
 
  This commit removes the warning in the case of lvalue vec, since it is
  similar to |=, but leaves it in place for rvalue vec.

:100644 100644 1bd16b5e668fdbd0d0be725e26625fd93cbf4051 b2b6546d8cd32bfffa1c65112697a04de9560090 M doop.c
:040000 040000 febb4034f8c7bc380a93193e5df0039f1e16c4d2 88120f8bce08edef4a05927b045a3a465087a857 M t
bisect run success
That took 793 seconds
--
Karl Williamson

@p5pRT
Copy link
Author

p5pRT commented Mar 24, 2014

From @tonycoz

On Mon Mar 03 11​:28​:21 2014, khw wrote​:

A bisect yields this​:

032061d is the first bad commit
commit 032061d
Author​: Brian Fraser <fraserbn@​gmail.com>
Date​: Sat May 26 17​:35​:26 2012 -0300

This is happening when using vec() to modify an undef sv, eg​:

$ valgrind -q ./perl -e 'vec($Foo, 0, 1) = 1'
==814== Conditional jump or move depends on uninitialised value(s)
==814== at 0x5190C7​: Perl_sv_pvn_force_flags (sv.c​:9505)
==814== by 0x580C9B​: Perl_do_vecset (doop.c​:927)
==814== by 0x4D30AD​: Perl_magic_setvec (mg.c​:2305)
==814== by 0x4CE522​: Perl_mg_set (mg.c​:279)
==814== by 0x4EB417​: Perl_pp_sassign (pp_hot.c​:223)
==814== by 0x4C42C9​: Perl_runops_debug (dump.c​:2425)
==814== by 0x44BEC4​: perl_run (perl.c​:2449)
==814== by 0x422934​: main (perlmain.c​:112)
==814==

The error is on this line​:

if (s != SvPVX_const(sv)) { /* Almost, but not quite, sv_setpvn() */
  if (SvROK(sv))
  sv_unref(sv);

where we examine sv->sv_u.svu_pv when it hasn't been initialized yet.

The following change prevents the error and passes all tests (DEBUGGING)​:

Inline Patch
diff --git a/sv.c b/sv.c
index dcb1d5e..4742c7d 100644
--- a/sv.c
+++ b/sv.c
@@ -9502,7 +9502,8 @@ Perl_sv_pvn_force_flags(pTHX_ SV *const sv, STRLEN *const 
        if (lp)
            *lp = len;
 
-       if (s != SvPVX_const(sv)) {     /* Almost, but not quite, sv_setpvn() */
+       if (SvTYPE(sv) < SVt_PV ||
+            s != SvPVX_const(sv)) {    /* Almost, but not quite, sv_setpvn() */
            if (SvROK(sv))
                sv_unref(sv);
            SvUPGRADE(sv, SVt_PV);              /* Never FALSE */

Tony

@p5pRT
Copy link
Author

p5pRT commented Mar 24, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Mar 25, 2014

From @shlomif

On Sun Mar 23 21​:15​:44 2014, tonyc wrote​:

On Mon Mar 03 11​:28​:21 2014, khw wrote​:

A bisect yields this​:

032061d is the first bad commit
commit 032061d
Author​: Brian Fraser <fraserbn@​gmail.com>
Date​: Sat May 26 17​:35​:26 2012 -0300

This is happening when using vec() to modify an undef sv, eg​:

$ valgrind -q ./perl -e 'vec($Foo, 0, 1) = 1'
==814== Conditional jump or move depends on uninitialised value(s)
==814== at 0x5190C7​: Perl_sv_pvn_force_flags (sv.c​:9505)
==814== by 0x580C9B​: Perl_do_vecset (doop.c​:927)
==814== by 0x4D30AD​: Perl_magic_setvec (mg.c​:2305)
==814== by 0x4CE522​: Perl_mg_set (mg.c​:279)
==814== by 0x4EB417​: Perl_pp_sassign (pp_hot.c​:223)
==814== by 0x4C42C9​: Perl_runops_debug (dump.c​:2425)
==814== by 0x44BEC4​: perl_run (perl.c​:2449)
==814== by 0x422934​: main (perlmain.c​:112)
==814==

The error is on this line​:

if (s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);

where we examine sv->sv_u.svu_pv when it hasn't been initialized yet.

The following change prevents the error and passes all tests
(DEBUGGING)​:

diff --git a/sv.c b/sv.c
index dcb1d5e..4742c7d 100644
--- a/sv.c
+++ b/sv.c
@​@​ -9502,7 +9502,8 @​@​ Perl_sv_pvn_force_flags(pTHX_ SV *const sv,
STRLEN *const
if (lp)
*lp = len;

- if (s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
+ if (SvTYPE(sv) < SVt_PV ||
+ s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);
SvUPGRADE(sv, SVt_PV); /* Never FALSE */

I wanted to test your patch but could not because it didn't apply cleanly using "patch -p1 < …". The problem seems to be that it is all whitespace, while the sources use a mixture of tabs and spaces. I can apply it manually and provide a new patch, but shouldn't the patch be attached?

Regards,

-- Shlomi Fish

@p5pRT
Copy link
Author

p5pRT commented Mar 25, 2014

From @shlomif

On Tue Mar 25 01​:24​:32 2014, shlomif wrote​:

On Sun Mar 23 21​:15​:44 2014, tonyc wrote​:

On Mon Mar 03 11​:28​:21 2014, khw wrote​:

A bisect yields this​:

032061d is the first bad commit
commit 032061d
Author​: Brian Fraser <fraserbn@​gmail.com>
Date​: Sat May 26 17​:35​:26 2012 -0300

This is happening when using vec() to modify an undef sv, eg​:

$ valgrind -q ./perl -e 'vec($Foo, 0, 1) = 1'
==814== Conditional jump or move depends on uninitialised value(s)
==814== at 0x5190C7​: Perl_sv_pvn_force_flags (sv.c​:9505)
==814== by 0x580C9B​: Perl_do_vecset (doop.c​:927)
==814== by 0x4D30AD​: Perl_magic_setvec (mg.c​:2305)
==814== by 0x4CE522​: Perl_mg_set (mg.c​:279)
==814== by 0x4EB417​: Perl_pp_sassign (pp_hot.c​:223)
==814== by 0x4C42C9​: Perl_runops_debug (dump.c​:2425)
==814== by 0x44BEC4​: perl_run (perl.c​:2449)
==814== by 0x422934​: main (perlmain.c​:112)
==814==

The error is on this line​:

if (s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);

where we examine sv->sv_u.svu_pv when it hasn't been initialized yet.

The following change prevents the error and passes all tests
(DEBUGGING)​:

diff --git a/sv.c b/sv.c
index dcb1d5e..4742c7d 100644
--- a/sv.c
+++ b/sv.c
@​@​ -9502,7 +9502,8 @​@​ Perl_sv_pvn_force_flags(pTHX_ SV *const sv,
STRLEN *const
if (lp)
*lp = len;

- if (s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
+ if (SvTYPE(sv) < SVt_PV ||
+ s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);
SvUPGRADE(sv, SVt_PV); /* Never FALSE */

I wanted to test your patch but could not because it didn't apply
cleanly using "patch -p1 < …". The problem seems to be that it is all
whitespace, while the sources use a mixture of tabs and spaces. I can
apply it manually and provide a new patch, but shouldn't the patch be
attached?

The reworked patch is attached to this comment. All tests are successful here (Mageia Linux x86-64 5/Cauldron).

Regards,

-- Shlomi Fish

@p5pRT
Copy link
Author

p5pRT commented Mar 25, 2014

From @shlomif

rt121366.diff
diff --git a/sv.c b/sv.c
index dcb1d5e..9afe983 100644
--- a/sv.c
+++ b/sv.c
@@ -9502,7 +9502,8 @@ Perl_sv_pvn_force_flags(pTHX_ SV *const sv, STRLEN *const lp, const I32 flags)
 	if (lp)
 	    *lp = len;
 
-	if (s != SvPVX_const(sv)) {	/* Almost, but not quite, sv_setpvn() */
+	if (SvTYPE(sv) < SVt_PV ||
+	    s != SvPVX_const(sv)) {	/* Almost, but not quite, sv_setpvn() */
 	    if (SvROK(sv))
 		sv_unref(sv);
 	    SvUPGRADE(sv, SVt_PV);		/* Never FALSE */

@p5pRT
Copy link
Author

p5pRT commented Mar 25, 2014

From @Hugmeir

On Mon, Mar 24, 2014 at 5​:15 AM, Tony Cook via RT
<perlbug-followup@​perl.org> wrote​:

On Mon Mar 03 11​:28​:21 2014, khw wrote​:

A bisect yields this​:

032061d is the first bad commit
commit 032061d
Author​: Brian Fraser <fraserbn@​gmail.com>
Date​: Sat May 26 17​:35​:26 2012 -0300

This is happening when using vec() to modify an undef sv, eg​:

$ valgrind -q ./perl -e 'vec($Foo, 0, 1) = 1'
==814== Conditional jump or move depends on uninitialised value(s)
==814== at 0x5190C7​: Perl_sv_pvn_force_flags (sv.c​:9505)
==814== by 0x580C9B​: Perl_do_vecset (doop.c​:927)
==814== by 0x4D30AD​: Perl_magic_setvec (mg.c​:2305)
==814== by 0x4CE522​: Perl_mg_set (mg.c​:279)
==814== by 0x4EB417​: Perl_pp_sassign (pp_hot.c​:223)
==814== by 0x4C42C9​: Perl_runops_debug (dump.c​:2425)
==814== by 0x44BEC4​: perl_run (perl.c​:2449)
==814== by 0x422934​: main (perlmain.c​:112)
==814==

The error is on this line​:

if \(s \!= SvPVX\_const\(sv\)\) \{     /\* Almost\, but not quite\, sv\_setpvn\(\) \*/
        if \(SvROK\(sv\)\)
            sv\_unref\(sv\);

where we examine sv->sv_u.svu_pv when it hasn't been initialized yet.

The following change prevents the error and passes all tests (DEBUGGING)​:

diff --git a/sv.c b/sv.c
index dcb1d5e..4742c7d 100644
--- a/sv.c
+++ b/sv.c
@​@​ -9502,7 +9502,8 @​@​ Perl_sv_pvn_force_flags(pTHX_ SV *const sv, STRLEN *const
if (lp)
*lp = len;

- if (s != SvPVX_const(sv)) { /* Almost, but not quite, sv_setpvn() */
+ if (SvTYPE(sv) < SVt_PV ||
+ s != SvPVX_const(sv)) { /* Almost, but not quite, sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);
SvUPGRADE(sv, SVt_PV); /* Never FALSE */

Tony

---
via perlbug​: queue​: perl5 status​: new
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121366

Thanks for picking up this, Tony! I haven't been able to get valgrind
or gdb to work on my Mac, so I've been stuck being useless for a
while.

@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2014

From @tonycoz

On Sun Mar 23 21​:15​:44 2014, tonyc wrote​:

This is happening when using vec() to modify an undef sv, eg​:

$ valgrind -q ./perl -e 'vec($Foo, 0, 1) = 1'
==814== Conditional jump or move depends on uninitialised value(s)
==814== at 0x5190C7​: Perl_sv_pvn_force_flags (sv.c​:9505)
==814== by 0x580C9B​: Perl_do_vecset (doop.c​:927)
==814== by 0x4D30AD​: Perl_magic_setvec (mg.c​:2305)
==814== by 0x4CE522​: Perl_mg_set (mg.c​:279)
==814== by 0x4EB417​: Perl_pp_sassign (pp_hot.c​:223)
==814== by 0x4C42C9​: Perl_runops_debug (dump.c​:2425)
==814== by 0x44BEC4​: perl_run (perl.c​:2449)
==814== by 0x422934​: main (perlmain.c​:112)
==814==

The error is on this line​:

if (s != SvPVX_const(sv)) { /* Almost, but not quite,
sv_setpvn() */
if (SvROK(sv))
sv_unref(sv);

where we examine sv->sv_u.svu_pv when it hasn't been initialized yet.

Pushed the fix as e141190.

Tony

@p5pRT
Copy link
Author

p5pRT commented Mar 26, 2014

@tonycoz - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant