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: pp_sys.c: uninitialized tmbuf #13758

Closed
p5pRT opened this issue Apr 23, 2014 · 12 comments
Closed

[PATCH] Coverity: pp_sys.c: uninitialized tmbuf #13758

p5pRT opened this issue Apr 23, 2014 · 12 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Apr 23, 2014

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

Searchable as RT121709$

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2014

From @jhi

Attached.

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2014

From @jhi

0006-Fix-for-Coverity-perl5-CID-29088.patch
From 978fb9a9aad85383b60c7a28506bab33941165f7 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Mon, 21 Apr 2014 17:53:13 -0400
Subject: [PATCH 6/9] Fix for Coverity perl5 CID 29088: Uninitialized scalar
 variable (UNINIT) uninit_use: Using uninitialized value tmbuf.tm_year.

There is a code path that can lead to accessing uninitialized tmbuf:
when the too-small or too-large time inputs to gmtime/localtime
happen.

- make it so that the tm_year is used only on successful code path:
  pp_sys.c
- add the gmtime failed / localtime failed errors to perldiag:
  pod/perldiag.pod
- test those errors: t/op/time.t
---
 pod/perldiag.pod | 10 ++++++++++
 pp_sys.c         | 27 +++++++++++++--------------
 t/op/time.t      |  9 ++++++---
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 00700c5..64d87a2 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -2128,6 +2128,11 @@ a term, so it's looking for the corresponding right angle bracket, and
 not finding it.  Chances are you left some needed parentheses out
 earlier in the line, and you really meant a "less than".
 
+=item gmtime(%f) failed
+
+(W overflow) You called C<gmtime> with a number that it could not handle:
+too large, too small, or NaN.  The returned value is C<undef>.
+
 =item gmtime(%f) too large
 
 (W overflow) You called C<gmtime> with a number that was larger than
@@ -2787,6 +2792,11 @@ L<perlfunc/listen>.
 form of C<open> does not support pipes, such as C<open($pipe, '|-', @args)>.
 Use the two-argument C<open($pipe, '|prog arg1 arg2...')> form instead.
 
+=item localtime(%f) failed
+
+(W overflow) You called C<localtime> with a number that it could not handle:
+too large, too small, or NaN.  The returned value is C<undef>.
+
 =item localtime(%f) too large
 
 (W overflow) You called C<localtime> with a number that was larger
diff --git a/pp_sys.c b/pp_sys.c
index 9f97177..43d6473 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -4483,30 +4483,29 @@ PP(pp_gmtime)
     }
 
     if (err == NULL) {
+	/* diag_listed_as: gmtime(%f) failed */
 	/* XXX %lld broken for quads */
 	Perl_ck_warner(aTHX_ packWARN(WARN_OVERFLOW),
 		       "%s(%.0" NVff ") failed", opname, when);
     }
 
     if (GIMME != G_ARRAY) {	/* scalar context */
-	SV *tsv;
-	/* XXX newSVpvf()'s %lld type is broken, so cheat with a double */
-	double year = (double)tmbuf.tm_year + 1900;
-
         EXTEND(SP, 1);
         EXTEND_MORTAL(1);
 	if (err == NULL)
 	    RETPUSHUNDEF;
-
-	tsv = Perl_newSVpvf(aTHX_ "%s %s %2d %02d:%02d:%02d %.0f",
-			    dayname[tmbuf.tm_wday],
-			    monname[tmbuf.tm_mon],
-			    tmbuf.tm_mday,
-			    tmbuf.tm_hour,
-			    tmbuf.tm_min,
-			    tmbuf.tm_sec,
-			    year);
-	mPUSHs(tsv);
+       else {
+           mPUSHs(Perl_newSVpvf(aTHX_ "%s %s %2d %02d:%02d:%02d %.0f",
+                                dayname[tmbuf.tm_wday],
+                                monname[tmbuf.tm_mon],
+                                tmbuf.tm_mday,
+                                tmbuf.tm_hour,
+                                tmbuf.tm_min,
+                                tmbuf.tm_sec,
+                                /* XXX newSVpvf()'s %lld type is broken,
+                                 * so cheat with a double */
+                                (double)tmbuf.tm_year + 1900));
+        }
     }
     else {			/* list context */
 	if ( err == NULL )
diff --git a/t/op/time.t b/t/op/time.t
index 4ac7f5b..734b838 100644
--- a/t/op/time.t
+++ b/t/op/time.t
@@ -6,7 +6,7 @@ BEGIN {
     require './test.pl';
 }
 
-plan tests => 66;
+plan tests => 70;
 
 # These tests make sure, among other things, that we don't end up
 # burning tons of CPU for dates far in the future.
@@ -210,7 +210,7 @@ SKIP: { #rt #73040
 	|| $small_time == $smallest
         || $big_time - 200 != $biggest
 	|| $big_time == $biggest) {
-	skip "Can't represent test values", 4;
+	skip "Can't represent test values", 8;
     }
     my $small_time_f = sprintf("%.0f", $small_time);
     my $big_time_f = sprintf("%.0f", $big_time);
@@ -221,17 +221,20 @@ SKIP: { #rt #73040
     $warning = '';
     my $date = gmtime($big_time);
     like $warning, qr/^gmtime\($big_time_f\) too large/;
+    like $warning, qr/^gmtime\($big_time_f\) failed/m;
 
     $warning = '';
     $date = localtime($big_time);
     like $warning, qr/^localtime\($big_time_f\) too large/;
+    like $warning, qr/^localtime\($big_time_f\) failed/m;
 
     $warning = '';
     $date = gmtime($small_time);
     like $warning, qr/^gmtime\($small_time_f\) too small/;
+    like $warning, qr/^gmtime\($small_time_f\) failed/m;
 
     $warning = '';
     $date = localtime($small_time);
     like $warning, qr/^localtime\($small_time_f\) too small/;
-  
+    like $warning, qr/^localtime\($small_time_f\) failed/m;
 }
-- 
1.8.5.2 (Apple Git-48)

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2014

From @tonycoz

On Tue Apr 22 17​:29​:56 2014, jhi wrote​:

Attached.

Added as a 5.21.1 blocker.

The extra $warning checks confused me until I saw we were generating two warnings for those calls to (local|gm)time

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 23, 2014

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

@p5pRT
Copy link
Author

p5pRT commented May 7, 2014

From @jhi

On Tuesday-201404-22, 20​:29, perlbug-followup@​perl.org wrote​:

Greetings,

This message has been automatically generated in response to the
creation of a perl bug report regarding​:
"[PATCH] Coverity​: pp_sys.c​: uninitialized tmbuf".

There is no need to reply to this message right now. Your ticket has been
assigned an ID of [perl #121709].

You can view your ticket at
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121709

Better summary line, patch attached.

@p5pRT
Copy link
Author

p5pRT commented May 7, 2014

From @jhi

0001-Uninitialized-tmbuf.patch
From 5500c3cf6201ccb9b4fe380e24f811f5cc018708 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Wed, 7 May 2014 09:26:52 -0400
Subject: [PATCH] Uninitialized tmbuf.

Fix for Coverity perl5 CID 29088: Uninitialized scalar variable (UNINIT)
uninit_use: Using uninitialized value tmbuf.tm_year.

There is a code path that can lead to accessing uninitialized tmbuf:
when the too-small or too-large time inputs to gmtime/localtime
happen.

- make it so that the tm_year is used only on successful code path:
  pp_sys.c
- add the gmtime failed / localtime failed errors to perldiag:
  pod/perldiag.pod
- test those errors: t/op/time.t
---
 pod/perldiag.pod | 10 ++++++++++
 pp_sys.c         | 27 +++++++++++++--------------
 t/op/time.t      |  9 ++++++---
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index f87ca9c..530bc47 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -2121,6 +2121,11 @@ a term, so it's looking for the corresponding right angle bracket, and
 not finding it.  Chances are you left some needed parentheses out
 earlier in the line, and you really meant a "less than".
 
+=item gmtime(%f) failed
+
+(W overflow) You called C<gmtime> with a number that it could not handle:
+too large, too small, or NaN.  The returned value is C<undef>.
+
 =item gmtime(%f) too large
 
 (W overflow) You called C<gmtime> with a number that was larger than
@@ -2780,6 +2785,11 @@ L<perlfunc/listen>.
 form of C<open> does not support pipes, such as C<open($pipe, '|-', @args)>.
 Use the two-argument C<open($pipe, '|prog arg1 arg2...')> form instead.
 
+=item localtime(%f) failed
+
+(W overflow) You called C<localtime> with a number that it could not handle:
+too large, too small, or NaN.  The returned value is C<undef>.
+
 =item localtime(%f) too large
 
 (W overflow) You called C<localtime> with a number that was larger
diff --git a/pp_sys.c b/pp_sys.c
index 9f97177..43d6473 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -4483,30 +4483,29 @@ PP(pp_gmtime)
     }
 
     if (err == NULL) {
+	/* diag_listed_as: gmtime(%f) failed */
 	/* XXX %lld broken for quads */
 	Perl_ck_warner(aTHX_ packWARN(WARN_OVERFLOW),
 		       "%s(%.0" NVff ") failed", opname, when);
     }
 
     if (GIMME != G_ARRAY) {	/* scalar context */
-	SV *tsv;
-	/* XXX newSVpvf()'s %lld type is broken, so cheat with a double */
-	double year = (double)tmbuf.tm_year + 1900;
-
         EXTEND(SP, 1);
         EXTEND_MORTAL(1);
 	if (err == NULL)
 	    RETPUSHUNDEF;
-
-	tsv = Perl_newSVpvf(aTHX_ "%s %s %2d %02d:%02d:%02d %.0f",
-			    dayname[tmbuf.tm_wday],
-			    monname[tmbuf.tm_mon],
-			    tmbuf.tm_mday,
-			    tmbuf.tm_hour,
-			    tmbuf.tm_min,
-			    tmbuf.tm_sec,
-			    year);
-	mPUSHs(tsv);
+       else {
+           mPUSHs(Perl_newSVpvf(aTHX_ "%s %s %2d %02d:%02d:%02d %.0f",
+                                dayname[tmbuf.tm_wday],
+                                monname[tmbuf.tm_mon],
+                                tmbuf.tm_mday,
+                                tmbuf.tm_hour,
+                                tmbuf.tm_min,
+                                tmbuf.tm_sec,
+                                /* XXX newSVpvf()'s %lld type is broken,
+                                 * so cheat with a double */
+                                (double)tmbuf.tm_year + 1900));
+        }
     }
     else {			/* list context */
 	if ( err == NULL )
diff --git a/t/op/time.t b/t/op/time.t
index 4ac7f5b..734b838 100644
--- a/t/op/time.t
+++ b/t/op/time.t
@@ -6,7 +6,7 @@ BEGIN {
     require './test.pl';
 }
 
-plan tests => 66;
+plan tests => 70;
 
 # These tests make sure, among other things, that we don't end up
 # burning tons of CPU for dates far in the future.
@@ -210,7 +210,7 @@ SKIP: { #rt #73040
 	|| $small_time == $smallest
         || $big_time - 200 != $biggest
 	|| $big_time == $biggest) {
-	skip "Can't represent test values", 4;
+	skip "Can't represent test values", 8;
     }
     my $small_time_f = sprintf("%.0f", $small_time);
     my $big_time_f = sprintf("%.0f", $big_time);
@@ -221,17 +221,20 @@ SKIP: { #rt #73040
     $warning = '';
     my $date = gmtime($big_time);
     like $warning, qr/^gmtime\($big_time_f\) too large/;
+    like $warning, qr/^gmtime\($big_time_f\) failed/m;
 
     $warning = '';
     $date = localtime($big_time);
     like $warning, qr/^localtime\($big_time_f\) too large/;
+    like $warning, qr/^localtime\($big_time_f\) failed/m;
 
     $warning = '';
     $date = gmtime($small_time);
     like $warning, qr/^gmtime\($small_time_f\) too small/;
+    like $warning, qr/^gmtime\($small_time_f\) failed/m;
 
     $warning = '';
     $date = localtime($small_time);
     like $warning, qr/^localtime\($small_time_f\) too small/;
-  
+    like $warning, qr/^localtime\($small_time_f\) failed/m;
 }
-- 
1.9.2

@p5pRT
Copy link
Author

p5pRT commented May 11, 2014

From @bulk88

On Wed May 07 06​:55​:52 2014, jhi wrote​:

On Tuesday-201404-22, 20​:29, perlbug-followup@​perl.org wrote​:

Greetings,

This message has been automatically generated in response to the
creation of a perl bug report regarding​:
"[PATCH] Coverity​: pp_sys.c​: uninitialized tmbuf".

There is no need to reply to this message right now. Your ticket has been
assigned an ID of [perl #121709].

You can view your ticket at
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121709

Better summary line, patch attached.

Why is this refactoring a bunch of lines unrelated to the uninit var access? the else block is redundant. By removing the tsv assignment it makes debugging harder.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @tonycoz

On Sat May 10 18​:49​:54 2014, bulk88 wrote​:

On Wed May 07 06​:55​:52 2014, jhi wrote​:

On Tuesday-201404-22, 20​:29, perlbug-followup@​perl.org wrote​:

Greetings,

This message has been automatically generated in response to the
creation of a perl bug report regarding​:
"[PATCH] Coverity​: pp_sys.c​: uninitialized tmbuf".

There is no need to reply to this message right now. Your ticket
has been
assigned an ID of [perl #121709].

You can view your ticket at
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121709

Better summary line, patch attached.

Why is this refactoring a bunch of lines unrelated to the uninit var
access? the else block is redundant. By removing the tsv assignment it
makes debugging harder.

If you need to debug the return value you can either use a debugger that gives access to the function return value, or temporarily modify the code to save the value, as has been discussed in other threads.

As to the re-org, while I can't read Jarkko's mind, it may be to move the declaration closer to the initialization, if we weren't restricted by certain older compilers that could be done in a simpler fashion.

Tony

@p5pRT
Copy link
Author

p5pRT commented May 13, 2014

From @jhi

On Monday-201405-12, 21​:50, Tony Cook via RT wrote​:

As to the re-org, while I can't read Jarkko's mind, it may be to move the declaration closer to the initialization, if we weren't restricted by certain older compilers that could be done in a simpler fashion.

If I could read my own mind...

I think it was something along the lines​:
(1) oh, tmbuf is used (and filled in) only in the success path...
(2) so let's move the tsv and year to be only in the success path, too
(3) hmm, now the tsv is kind of one-use-only anymore
(4) therefore we can just directly mPUSHs the result of pvf()
(5) and lookie, we can inline the year computation, too

(to recap what Coverity noticed​: in the failure path the
tmbuf remained uninitialized (*), and then the computation of year tried
to use that.)

(*) I simply cannot type the word straight​: unitial... unitializ... Darn
it. Ditto derenf... derefenc... dereferefenced. That one.

@p5pRT
Copy link
Author

p5pRT commented May 29, 2014

From @tonycoz

On Wed May 07 06​:55​:52 2014, jhi wrote​:

On Tuesday-201404-22, 20​:29, perlbug-followup@​perl.org wrote​:

Greetings,

This message has been automatically generated in response to the
creation of a perl bug report regarding​:
"[PATCH] Coverity​: pp_sys.c​: uninitialized tmbuf".

There is no need to reply to this message right now. Your ticket has been
assigned an ID of [perl #121709].

You can view your ticket at
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121709

Better summary line, patch attached.

Thanks, applied as b35b96b.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 1, 2014

From @jkeenan

No failure reports to date; marking ticket Resolved.

@p5pRT p5pRT closed this as completed Jun 1, 2014
@p5pRT
Copy link
Author

p5pRT commented Jun 1, 2014

@jkeenan - 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