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

[PATCH] Coverity: ptr values immediately discarded #13777

Closed
p5pRT opened this issue Apr 26, 2014 · 19 comments
Closed

[PATCH] Coverity: ptr values immediately discarded #13777

p5pRT opened this issue Apr 26, 2014 · 19 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Apr 26, 2014

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

Searchable as RT121747$

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2014

From @jhi

Attached.

@p5pRT
Copy link
Author

p5pRT commented Apr 26, 2014

From @jhi

0001-Fix-for-Coverity-perl5-CIDs-29203-29207-29211-29214-.patch
From de98b5381592deb2f2569caca8501ff8815c1d0e Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Fri, 25 Apr 2014 21:52:54 -0400
Subject: [PATCH] Fix for Coverity perl5 CIDs 29203, 29207, 29211, 29214,
 29217, 29218, 29222: Unused pointer value (UNUSED_VALUE) Pointer foo returned
 by bar() is overwritten.

Pointers set but then (immediately or very shortly) overwritten.
---
 ext/XS-APItest/APItest.xs | 4 ++--
 perlio.c                  | 6 +++---
 toke.c                    | 2 +-
 x2p/a2py.c                | 2 +-
 x2p/walk.c                | 6 +++---
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs
index a51924d..f741664 100644
--- a/ext/XS-APItest/APItest.xs
+++ b/ext/XS-APItest/APItest.xs
@@ -1457,10 +1457,10 @@ common(params)
 	if ((svp = hv_fetchs(params, "hash", 0)))
 	    hash = SvUV(*svp);
 
-	if ((svp = hv_fetchs(params, "hash_pv", 0))) {
+	if (hv_fetchs(params, "hash_pv", 0)) {
 	    PERL_HASH(hash, key, klen);
 	}
-	if ((svp = hv_fetchs(params, "hash_sv", 0))) {
+	if (hv_fetchs(params, "hash_sv", 0)) {
 	    STRLEN len;
 	    const char *const p = SvPV(keysv, len);
 	    PERL_HASH(hash, p, len);
diff --git a/perlio.c b/perlio.c
index 0ae0a43..472899c 100644
--- a/perlio.c
+++ b/perlio.c
@@ -2945,7 +2945,7 @@ PerlIO_importFILE(FILE *stdio, const char *mode)
 	    }
 	    fclose(f2);
 	}
-	if ((f = PerlIO_push(aTHX_(f = PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
+	if ((f = PerlIO_push(aTHX_(PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
 	    s = PerlIOSelf(f, PerlIOStdio);
 	    s->stdio = stdio;
 	    PerlIOUnix_refcnt_inc(fileno(stdio));
@@ -2968,8 +2968,8 @@ PerlIOStdio_open(pTHX_ PerlIO_funcs *self, PerlIO_list_t *layers,
 	if (!IS_SAFE_PATHNAME(path, len, "open"))
             return NULL;
 	PerlIOUnix_refcnt_dec(fileno(s->stdio));
-	stdio = PerlSIO_freopen(path, (mode = PerlIOStdio_mode(mode, tmode)),
-			    s->stdio);
+	stdio = PerlSIO_freopen(path, PerlIOStdio_mode(mode, tmode),
+                                s->stdio);
 	if (!s->stdio)
 	    return NULL;
 	s->stdio = stdio;
diff --git a/toke.c b/toke.c
index 3d992f6..8a560aa 100644
--- a/toke.c
+++ b/toke.c
@@ -10398,7 +10398,7 @@ S_scan_inputsymbol(pTHX_ char *start)
 	    Copy("ARGV",d,5,char);
 
 	/* Check whether readline() is overriden */
-	gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
+	gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
 	if ((gv_readline = gv_override("readline",8)))
 	    readline_overriden = TRUE;
 
diff --git a/x2p/a2py.c b/x2p/a2py.c
index 8c08202..7df6929 100644
--- a/x2p/a2py.c
+++ b/x2p/a2py.c
@@ -70,7 +70,7 @@ main(int argc, const char **argv)
 
     myname = argv[0];
     linestr = str_new(80);
-    str = str_new(0);		/* first used for -I flags */
+    str_new(0);		/* first used for -I flags */
     for (argc--,argv++; argc; argc--,argv++) {
 	if (argv[0][0] != '-' || !argv[0][1])
 	    break;
diff --git a/x2p/walk.c b/x2p/walk.c
index 0197fea..be27218 100644
--- a/x2p/walk.c
+++ b/x2p/walk.c
@@ -254,7 +254,7 @@ sub Pick {\n\
 	break;
     case OHUNK:
 	if (len == 1) {
-	    str = str_new(0);
+	    str_new(0);
 	    str = walk(0,level,oper1(OPRINT,0),&numarg,P_MIN);
 	    str_cat(str," if ");
 	    str_scat(str,fstr=walk(0,level,ops[node+1].ival,&numarg,P_MIN));
@@ -264,7 +264,7 @@ sub Pick {\n\
 	else {
 	    tmpstr = walk(0,level,ops[node+1].ival,&numarg,P_MIN);
 	    if (*tmpstr->str_ptr) {
-		str = str_new(0);
+		str_new(0);
 		str_set(str,"if (");
 		str_scat(str,tmpstr);
 		str_cat(str,") {\n");
@@ -1104,7 +1104,7 @@ sub Pick {\n\
 	if (!do_fancy_opens) {
 	    t = tmpstr->str_ptr;
 	    if (*t == '"' || *t == '\'')
-		t = cpytill(tokenbuf,t+1,*t);
+		cpytill(tokenbuf,t+1,*t);
 	    else
 		fatal("Internal error: OCLOSE %s",t);
 	    s = savestr(tokenbuf);
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @tonycoz

On Sat Apr 26 13​:03​:54 2014, jhi wrote​:

Attached.

--- a/toke.c
+++ b/toke.c
@​@​ -10398,7 +10398,7 @​@​ S_scan_inputsymbol(pTHX_ char *start)
  Copy("ARGV",d,5,char);

  /* Check whether readline() is overriden */
- gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
+ gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
  if ((gv_readline = gv_override("readline",8)))
  readline_overriden = TRUE;

I think that line can just be removed. The first thing gv_override() does is​:

  GV *gv = gv_fetchpvn(name, len, GV_NOTQUAL, SVt_PVCV);

--- a/x2p/a2py.c
+++ b/x2p/a2py.c
@​@​ -70,7 +70,7 @​@​ main(int argc, const char **argv)

  myname = argv[0];
  linestr = str_new(80);
- str = str_new(0); /* first used for -I flags */
+ str_new(0); /* first used for -I flags */
  for (argc--,argv++; argc; argc--,argv++) {
  if (argv[0][0] != '-' || !argv[0][1])
  break;

Should simply be removed, str_new() is an allocator that pulls from a free list if it can.

--- a/x2p/walk.c
+++ b/x2p/walk.c
@​@​ -254,7 +254,7 @​@​ sub Pick {\n\
  break;
  case OHUNK​:
  if (len == 1) {
- str = str_new(0);
+ str_new(0);
  str = walk(0,level,oper1(OPRINT,0),&numarg,P_MIN);
  str_cat(str," if ");
  str_scat(str,fstr=walk(0,level,ops[node+1].ival,&numarg,P_MIN));

Again.

@​@​ -264,7 +264,7 @​@​ sub Pick {\n\
  else {
  tmpstr = walk(0,level,ops[node+1].ival,&numarg,P_MIN);
  if (*tmpstr->str_ptr) {
- str = str_new(0);
+ str_new(0);
  str_set(str,"if (");
  str_scat(str,tmpstr);
  str_cat(str,") {\n");

This one looks wrong - str is used on the next line.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @bulk88

On Sat Apr 26 13​:03​:54 2014, jhi wrote​:

Attached.

I disagree with patch. When step debugging -O0/-Od code, seeing the c auto assignment which will appear in a "watch" area is important. On optimized code, the assignments and/or c auto vars are optimized away so the assignments dont hurt.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @jhi

On Monday-201404-28, 3​:11, bulk88 via RT wrote​:

I disagree with patch.

When step debugging -O0/-Od code, seeing the c auto assignment which
will appear in a "watch" area is important.

I don't understand. How is seeing an assignment that doesn't
effectively do anything important? The RHS is still being called
(unless it was already dead code, of course). If you want to break on
that line, break on that line.

On optimized code, the assignments and/or c auto vars are optimized away
so the assignments dont hurt.

I don't think this is a good trade-off. Dead code is confusing to the
reader of the code.

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @jhi

- gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
+ gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
if ((gv_readline = gv_override("readline",8)))
readline_overriden = TRUE;

I think that line can just be removed. The first thing gv_override() does is​:

 GV \*gv = gv\_fetchpvn\(name\, len\, GV\_NOTQUAL\, SVt\_PVCV\);

Discarded.

- str = str_new(0); /* first used for -I flags */
+ str_new(0); /* first used for -I flags */
Should simply be removed, str_new() is an allocator that pulls from a free list if it can.

Removed. Left the comment though.

- str = str_new(0);
+ str_new(0);
Again.

Ditto.

@​@​ -264,7 +264,7 @​@​ sub Pick {\n\
else {
tmpstr = walk(0,level,ops[node+1].ival,&numarg,P_MIN);
if (*tmpstr->str_ptr) {
- str = str_new(0);
+ str_new(0);
str_set(str,"if (");
str_scat(str,tmpstr);
str_cat(str,") {\n");

This one looks wrong - str is used on the next line.

My bad.

Thanks for the review of my patches, by the way! I'm a bit rusty in
this business.

Refreshed patch attached.

@p5pRT
Copy link
Author

p5pRT commented Apr 28, 2014

From @jhi

0001-Fix-for-Coverity-perl5-CIDs-29203-29207-29211-29214-.patch
From 7200962f3f298ed375b7b4127be8c4aea5ef783d Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Fri, 25 Apr 2014 21:52:54 -0400
Subject: [PATCH] Fix for Coverity perl5 CIDs 29203, 29207, 29211, 29214,
 29217, 29218, 29222: Unused pointer value (UNUSED_VALUE) Pointer foo returned
 by bar() is overwritten.

Pointers set but then (immediately or very shortly) overwritten.
---
 ext/XS-APItest/APItest.xs | 4 ++--
 perlio.c                  | 6 +++---
 toke.c                    | 1 -
 x2p/a2py.c                | 2 +-
 x2p/walk.c                | 3 +--
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs
index a51924d..f741664 100644
--- a/ext/XS-APItest/APItest.xs
+++ b/ext/XS-APItest/APItest.xs
@@ -1457,10 +1457,10 @@ common(params)
 	if ((svp = hv_fetchs(params, "hash", 0)))
 	    hash = SvUV(*svp);
 
-	if ((svp = hv_fetchs(params, "hash_pv", 0))) {
+	if (hv_fetchs(params, "hash_pv", 0)) {
 	    PERL_HASH(hash, key, klen);
 	}
-	if ((svp = hv_fetchs(params, "hash_sv", 0))) {
+	if (hv_fetchs(params, "hash_sv", 0)) {
 	    STRLEN len;
 	    const char *const p = SvPV(keysv, len);
 	    PERL_HASH(hash, p, len);
diff --git a/perlio.c b/perlio.c
index 0ae0a43..472899c 100644
--- a/perlio.c
+++ b/perlio.c
@@ -2945,7 +2945,7 @@ PerlIO_importFILE(FILE *stdio, const char *mode)
 	    }
 	    fclose(f2);
 	}
-	if ((f = PerlIO_push(aTHX_(f = PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
+	if ((f = PerlIO_push(aTHX_(PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
 	    s = PerlIOSelf(f, PerlIOStdio);
 	    s->stdio = stdio;
 	    PerlIOUnix_refcnt_inc(fileno(stdio));
@@ -2968,8 +2968,8 @@ PerlIOStdio_open(pTHX_ PerlIO_funcs *self, PerlIO_list_t *layers,
 	if (!IS_SAFE_PATHNAME(path, len, "open"))
             return NULL;
 	PerlIOUnix_refcnt_dec(fileno(s->stdio));
-	stdio = PerlSIO_freopen(path, (mode = PerlIOStdio_mode(mode, tmode)),
-			    s->stdio);
+	stdio = PerlSIO_freopen(path, PerlIOStdio_mode(mode, tmode),
+                                s->stdio);
 	if (!s->stdio)
 	    return NULL;
 	s->stdio = stdio;
diff --git a/toke.c b/toke.c
index 3d992f6..a44a8b7 100644
--- a/toke.c
+++ b/toke.c
@@ -10398,7 +10398,6 @@ S_scan_inputsymbol(pTHX_ char *start)
 	    Copy("ARGV",d,5,char);
 
 	/* Check whether readline() is overriden */
-	gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
 	if ((gv_readline = gv_override("readline",8)))
 	    readline_overriden = TRUE;
 
diff --git a/x2p/a2py.c b/x2p/a2py.c
index 8c08202..8daee2a 100644
--- a/x2p/a2py.c
+++ b/x2p/a2py.c
@@ -70,7 +70,7 @@ main(int argc, const char **argv)
 
     myname = argv[0];
     linestr = str_new(80);
-    str = str_new(0);		/* first used for -I flags */
+    /* first used for -I flags */
     for (argc--,argv++; argc; argc--,argv++) {
 	if (argv[0][0] != '-' || !argv[0][1])
 	    break;
diff --git a/x2p/walk.c b/x2p/walk.c
index 0197fea..d6962ed 100644
--- a/x2p/walk.c
+++ b/x2p/walk.c
@@ -254,7 +254,6 @@ sub Pick {\n\
 	break;
     case OHUNK:
 	if (len == 1) {
-	    str = str_new(0);
 	    str = walk(0,level,oper1(OPRINT,0),&numarg,P_MIN);
 	    str_cat(str," if ");
 	    str_scat(str,fstr=walk(0,level,ops[node+1].ival,&numarg,P_MIN));
@@ -1104,7 +1103,7 @@ sub Pick {\n\
 	if (!do_fancy_opens) {
 	    t = tmpstr->str_ptr;
 	    if (*t == '"' || *t == '\'')
-		t = cpytill(tokenbuf,t+1,*t);
+		cpytill(tokenbuf,t+1,*t);
 	    else
 		fatal("Internal error: OCLOSE %s",t);
 	    s = savestr(tokenbuf);
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2014

From @bulk88

On Mon Apr 28 04​:15​:01 2014, jhi wrote​:

On Monday-201404-28, 3​:11, bulk88 via RT wrote​:

I disagree with patch.

When step debugging -O0/-Od code, seeing the c auto assignment which
will appear in a "watch" area is important.

I don't understand. How is seeing an assignment that doesn't
effectively do anything important? The RHS is still being called
(unless it was already dead code, of course). If you want to break on
that line, break on that line.

How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?

On optimized code, the assignments and/or c auto vars are optimized away
so the assignments dont hurt.

I don't think this is a good trade-off. Dead code is confusing to the
reader of the code.

What is the value of slice tcode? and you can not recompile the code or make the C debugger call sv_dump on demand or look at asm code and registers to figure it out

av_store(av, 0 newSVsv(*hv_fetch(hv, "tcode", sizeof("tcode")-1, 1)));

or

startPerlTranscact(pTHX_ hv) {
  hv_fetch(hv, "started", sizeof("started")-1, 1);
  startTransact(SvIVx(*hv_fetch(hv, "ptr", sizeof("ptr")-1, 0))); /* SEGV intentional */
}

Was it in progress already? startTransact() doesn't care, but maybe you do, unless you save the result of hv_fetch to SV ** "svp" and get a "ptr values immediately discarded" warning, you won't know if it was started already unless you stepped through hv_common, and stepping through hv_common costs alot of human time.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2014

From @iabyn

On Tue, Apr 29, 2014 at 11​:04​:00PM -0700, bulk88 via RT wrote​:

On Mon Apr 28 04​:15​:01 2014, jhi wrote​:

On Monday-201404-28, 3​:11, bulk88 via RT wrote​:

I disagree with patch.

When step debugging -O0/-Od code, seeing the c auto assignment which
will appear in a "watch" area is important.

I don't understand. How is seeing an assignment that doesn't
effectively do anything important? The RHS is still being called
(unless it was already dead code, of course). If you want to break on
that line, break on that line.

How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?

This is silly. How would you debug code like

  if (foo() || bar()) ....
or
  (void)foo(....);
or
  foo(bar());

Would you ban all such uses from the perl core? There are thousands of
places in the perl core where function calls don't assign their return
value to an explicit local var.

If I were to debug such a statement using gdb say, I might single-step
into the function, then do 'finish' to run to the end of the function. gdb
will then automatically display the return value of the function.

If I needed to do a lot of debugging on that statement, I might
temporarily edit the code to add an intermediate variable.

--
"There's something wrong with our bloody ships today, Chatfield."
  -- Admiral Beatty at the Battle of Jutland, 31st May 1916.

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2014

From @steve-m-hay

On 30 April 2014 07​:04, bulk88 via RT <perlbug-followup@​perl.org> wrote​:

On Mon Apr 28 04​:15​:01 2014, jhi wrote​:

On Monday-201404-28, 3​:11, bulk88 via RT wrote​:

I disagree with patch.

When step debugging -O0/-Od code, seeing the c auto assignment which
will appear in a "watch" area is important.

I don't understand. How is seeing an assignment that doesn't
effectively do anything important? The RHS is still being called
(unless it was already dead code, of course). If you want to break on
that line, break on that line.

How are you doing to see the returned value of the function in a debugger even if you break on the line if its not assigned to a var?

In Dev Studio (2010, at least, I've not looked at earlier versions for
a long time), if you break on a function call and then step over it
then the return value of the function is shown in the Autos window,
with the Name column (normally containing a variable's name) being
"<function> returned" and the Value column being the <function>'s
return value.

@p5pRT
Copy link
Author

p5pRT commented Apr 30, 2014

From @bulk88

On Wed Apr 30 08​:55​:06 2014, davem wrote​:

This is silly. How would you debug code like

if (foo() || bar()) ....
or
(void)foo(....);
or
foo(bar());

Would you ban all such uses from the perl core? There are thousands
of
places in the perl core where function calls don't assign their return
value to an explicit local var.

If saving it to an auto is already there, we shouldn't be removing it just to satisfy a tool's policies. I am not calling for adding it everywhere to the core, just not removing it if its already there for some reason. Also if for some reason, the return value needs to be used in the future, the name of the var to hold the returned value is already known and a new name doesn't have to be thought up of.

On Wed Apr 30 10​:03​:33 2014, shay wrote​:

In Dev Studio (2010, at least, I've not looked at earlier versions for
a long time), if you break on a function call and then step over it
then the return value of the function is shown in the Autos window,
with the Name column (normally containing a variable's name) being
"<function> returned" and the Value column being the <function>'s
return value.

I didn't want to discuss VS IDE specifically in this ticket, but I know of the Autos window, but I never keep it on the screen, since it will show the value for exactly 1 line, not 2 or 3 lines. 1 click too many and its time to restart the app. I find the Auto tab to be almost useless. Locals tab is what I keep on screen.

--
bulk88 ~ bulk88 at hotmail.com

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 5, 2014

From @tonycoz

On Mon Apr 28 04​:30​:00 2014, jhi wrote​:

- str = str_new(0); /* first used for -I flags */
+ str_new(0); /* first used for -I flags */
Should simply be removed, str_new() is an allocator that pulls from a
free list if it can.

Removed. Left the comment though.

I'm not sure the comment ever applied - I don't see any handling of a -I flag, even back in perl 1.0.

I don't see any other issues.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 6, 2014

From @jhi

Refreshed patch attached.

Found one more similar spot, in toke.c​:10260-ish

  s = lex_grow_linestr(...);

the s was very shortly overwritten.

Refreshed patch attached.

@p5pRT
Copy link
Author

p5pRT commented May 6, 2014

From @jhi

0001-Pointers-set-but-then-immediately-or-shortly-overwri.patch
From b2f14f47156343d9647ef9d9190d9bffbab10c52 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Fri, 25 Apr 2014 21:52:54 -0400
Subject: [PATCH] Pointers set but then (immediately or shortly) overwritten.

Coverity perl5 CIDs 29202, 29203, 29207, 29211, 29214, 29217, 29218, 29222:
Unused pointer value (UNUSED_VALUE)
Pointer foo returned by bar() is overwritten.
---
 ext/XS-APItest/APItest.xs | 4 ++--
 perlio.c                  | 6 +++---
 toke.c                    | 3 +--
 x2p/a2py.c                | 2 +-
 x2p/walk.c                | 3 +--
 5 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/ext/XS-APItest/APItest.xs b/ext/XS-APItest/APItest.xs
index a51924d..f741664 100644
--- a/ext/XS-APItest/APItest.xs
+++ b/ext/XS-APItest/APItest.xs
@@ -1457,10 +1457,10 @@ common(params)
 	if ((svp = hv_fetchs(params, "hash", 0)))
 	    hash = SvUV(*svp);
 
-	if ((svp = hv_fetchs(params, "hash_pv", 0))) {
+	if (hv_fetchs(params, "hash_pv", 0)) {
 	    PERL_HASH(hash, key, klen);
 	}
-	if ((svp = hv_fetchs(params, "hash_sv", 0))) {
+	if (hv_fetchs(params, "hash_sv", 0)) {
 	    STRLEN len;
 	    const char *const p = SvPV(keysv, len);
 	    PERL_HASH(hash, p, len);
diff --git a/perlio.c b/perlio.c
index 0ae0a43..472899c 100644
--- a/perlio.c
+++ b/perlio.c
@@ -2945,7 +2945,7 @@ PerlIO_importFILE(FILE *stdio, const char *mode)
 	    }
 	    fclose(f2);
 	}
-	if ((f = PerlIO_push(aTHX_(f = PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
+	if ((f = PerlIO_push(aTHX_(PerlIO_allocate(aTHX)), PERLIO_FUNCS_CAST(&PerlIO_stdio), mode, NULL))) {
 	    s = PerlIOSelf(f, PerlIOStdio);
 	    s->stdio = stdio;
 	    PerlIOUnix_refcnt_inc(fileno(stdio));
@@ -2968,8 +2968,8 @@ PerlIOStdio_open(pTHX_ PerlIO_funcs *self, PerlIO_list_t *layers,
 	if (!IS_SAFE_PATHNAME(path, len, "open"))
             return NULL;
 	PerlIOUnix_refcnt_dec(fileno(s->stdio));
-	stdio = PerlSIO_freopen(path, (mode = PerlIOStdio_mode(mode, tmode)),
-			    s->stdio);
+	stdio = PerlSIO_freopen(path, PerlIOStdio_mode(mode, tmode),
+                                s->stdio);
 	if (!s->stdio)
 	    return NULL;
 	s->stdio = stdio;
diff --git a/toke.c b/toke.c
index 3d992f6..1d2ea9d 100644
--- a/toke.c
+++ b/toke.c
@@ -10257,7 +10257,7 @@ S_scan_heredoc(pTHX_ char *s)
 	}
 	CopLINE_set(PL_curcop, origline);
 	if (!SvCUR(PL_linestr) || PL_bufend[-1] != '\n') {
-            s = lex_grow_linestr(SvLEN(PL_linestr) + 3);
+            lex_grow_linestr(SvLEN(PL_linestr) + 3);
             /* ^That should be enough to avoid this needing to grow:  */
 	    sv_catpvs(PL_linestr, "\n\0");
             assert(s == SvPVX(PL_linestr));
@@ -10398,7 +10398,6 @@ S_scan_inputsymbol(pTHX_ char *start)
 	    Copy("ARGV",d,5,char);
 
 	/* Check whether readline() is overriden */
-	gv_readline = gv_fetchpvs("readline", GV_NOTQUAL, SVt_PVCV);
 	if ((gv_readline = gv_override("readline",8)))
 	    readline_overriden = TRUE;
 
diff --git a/x2p/a2py.c b/x2p/a2py.c
index 8c08202..8daee2a 100644
--- a/x2p/a2py.c
+++ b/x2p/a2py.c
@@ -70,7 +70,7 @@ main(int argc, const char **argv)
 
     myname = argv[0];
     linestr = str_new(80);
-    str = str_new(0);		/* first used for -I flags */
+    /* first used for -I flags */
     for (argc--,argv++; argc; argc--,argv++) {
 	if (argv[0][0] != '-' || !argv[0][1])
 	    break;
diff --git a/x2p/walk.c b/x2p/walk.c
index 0197fea..d6962ed 100644
--- a/x2p/walk.c
+++ b/x2p/walk.c
@@ -254,7 +254,6 @@ sub Pick {\n\
 	break;
     case OHUNK:
 	if (len == 1) {
-	    str = str_new(0);
 	    str = walk(0,level,oper1(OPRINT,0),&numarg,P_MIN);
 	    str_cat(str," if ");
 	    str_scat(str,fstr=walk(0,level,ops[node+1].ival,&numarg,P_MIN));
@@ -1104,7 +1103,7 @@ sub Pick {\n\
 	if (!do_fancy_opens) {
 	    t = tmpstr->str_ptr;
 	    if (*t == '"' || *t == '\'')
-		t = cpytill(tokenbuf,t+1,*t);
+		cpytill(tokenbuf,t+1,*t);
 	    else
 		fatal("Internal error: OCLOSE %s",t);
 	    s = savestr(tokenbuf);
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented May 7, 2014

From @tonycoz

On Tue May 06 13​:04​:57 2014, jhi wrote​:

Refreshed patch attached.

Found one more similar spot, in toke.c​:10260-ish

s = lex_grow_linestr(...);

the s was very shortly overwritten.

Refreshed patch attached.

@​@​ -10257,7 +10257,7 @​@​ S_scan_heredoc(pTHX_ char *s)
  }
  CopLINE_set(PL_curcop, origline);
  if (!SvCUR(PL_linestr) || PL_bufend[-1] != '\n') {
- s = lex_grow_linestr(SvLEN(PL_linestr) + 3);
+ lex_grow_linestr(SvLEN(PL_linestr) + 3);
  /* ^That should be enough to avoid this needing to grow​: */
  sv_catpvs(PL_linestr, "\n\0");

In debugging builds s is used before it's overwritten​:

  assert(s == SvPVX(PL_linestr));

Nicholas added both the assignment and the assert() in d8fe30a.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 7, 2014

From @jhi

In debugging builds s is used before it's overwritten​:

          assert\(s == SvPVX\(PL\_linestr\)\);

Nicholas added both the assignment and the assert() in d8fe30a.

Urgnkh. Please discard the latest revision, and prefer the one before
that, then.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 29, 2014

From @tonycoz

On Mon Apr 28 04​:30​:00 2014, jhi wrote​:

Refreshed patch attached.

Thanks, applied as de09213 with a subject edit.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 29, 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
Labels
Projects
None yet
Development

No branches or pull requests

1 participant