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
Comments
From @jhiAttached. |
From @jhi0001-Fix-for-Coverity-perl5-CIDs-29203-29207-29211-29214-.patchFrom 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
|
From @tonycozOn Sat Apr 26 13:03:54 2014, jhi wrote:
--- a/toke.c /* Check whether readline() is overriden */ 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 myname = argv[0]; Should simply be removed, str_new() is an allocator that pulls from a free list if it can. --- a/x2p/walk.c Again. @@ -264,7 +264,7 @@ sub Pick {\n\ This one looks wrong - str is used on the next line. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Sat Apr 26 13:03:54 2014, jhi 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. On optimized code, the assignments and/or c auto vars are optimized away so the assignments dont hurt. -- |
From @jhiOn Monday-201404-28, 3:11, bulk88 via RT wrote:
I don't understand. How is seeing an assignment that doesn't On optimized code, the assignments and/or c auto vars are optimized away I don't think this is a good trade-off. Dead code is confusing to the |
From @jhi
Discarded.
Removed. Left the comment though.
Ditto.
My bad. Thanks for the review of my patches, by the way! I'm a bit rusty in Refreshed patch attached. |
From @jhi0001-Fix-for-Coverity-perl5-CIDs-29203-29207-29211-29214-.patchFrom 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
|
From @bulk88On Mon Apr 28 04:15:01 2014, jhi wrote:
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?
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) { 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. -- |
From @iabynOn Tue, Apr 29, 2014 at 11:04:00PM -0700, bulk88 via RT wrote:
This is silly. How would you debug code like if (foo() || bar()) .... Would you ban all such uses from the perl core? There are thousands of If I were to debug such a statement using gdb say, I might single-step If I needed to do a lot of debugging on that statement, I might -- |
From @steve-m-hayOn 30 April 2014 07:04, bulk88 via RT <perlbug-followup@perl.org> wrote:
In Dev Studio (2010, at least, I've not looked at earlier versions for |
From @bulk88On Wed Apr 30 08:55:06 2014, davem wrote:
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:
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. -- -- |
From @tonycozOn Mon Apr 28 04:30:00 2014, jhi wrote:
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 |
From @jhi
Found one more similar spot, in toke.c:10260-ish s = lex_grow_linestr(...); the s was very shortly overwritten. Refreshed patch attached. |
From @jhi0001-Pointers-set-but-then-immediately-or-shortly-overwri.patchFrom 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
|
From @tonycozOn Tue May 06 13:04:57 2014, jhi wrote:
@@ -10257,7 +10257,7 @@ S_scan_heredoc(pTHX_ char *s) 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 |
@tonycoz - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#121747 (status was 'resolved')
Searchable as RT121747$
The text was updated successfully, but these errors were encountered: