Bug 120823 - Crash if attempt to read past end of dirfile.
Summary: Crash if attempt to read past end of dirfile.
Status: RESOLVED FIXED
Alias: None
Product: kst
Classification: Applications
Component: general (show other bugs)
Version: 1.x
Platform: unspecified Linux
: NOR crash
Target Milestone: ---
Assignee: kst
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-01-26 18:19 UTC by Netterfield
Modified: 2006-01-28 01:36 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Netterfield 2006-01-26 18:19:26 UTC
Version:           1.2.0_devel (using KDE 3.4.3, Gentoo)
Compiler:          gcc version 3.3.6 (Gentoo 3.3.6, ssp-3.3.6-1.0, pie-8.7.8)
OS:                Linux (i686) release 2.6.14-gentoo-r5

Load a dirfile (eg, from dirfile_maker).

In the wizard, load fields.  Put the starting frame past the end of the file.

When you apply, kst crashes with the following assertion failure:

kst: /home/cbn/programs/KDE/graphics/kst/kst/kstrvector.cpp:579: KstObject::UpdateType KstRVector::doUpdate(bool): Assertion `new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1' failed.
Comment 1 George Staikos 2006-01-26 18:31:40 UTC
> kst: /home/cbn/programs/KDE/graphics/kst/kst/kstrvector.cpp:579:
> KstObject::UpdateType KstRVector::doUpdate(bool): Assertion `new_nf - NF -
> 1 > 0 || new_nf - NF - 1 == -1' failed.


  Do you know what the values of new_nf and NF are?  (can you find out?)
Comment 2 Andrew Walker 2006-01-26 19:08:19 UTC
new_nf 1
NF     0
new_f0 0
Comment 3 Andrew Walker 2006-01-26 19:39:00 UTC
It would appear that the asserts are invalid, and that the subsequent _file->readField(...) calls should be protected by some if statements checking for the values of new_nf, NF, and new_f0 (as is the case for the samplesPerFrame = 1).
Comment 4 George Staikos 2006-01-26 20:07:35 UTC
On Thursday 26 January 2006 13:39, Andrew Walker wrote:
> ------- It would appear that the asserts are invalid, and that the
> subsequent _file->readField(...) calls should be protected by some if
> statements checking for the values of new_nf, NF, and new_f0 (as is the
> case for the samplesPerFrame = 1).


   Why?
Comment 5 Andrew Walker 2006-01-26 20:10:23 UTC
The ascii data source doesn't crash under the same conditions and the only difference is the samplesPerFrame value. It seems clear that the asserts are unnecessarily causing a crash.
Comment 6 George Staikos 2006-01-26 20:18:09 UTC
On Thursday 26 January 2006 14:10, Andrew Walker wrote:
> ------- The ascii data source doesn't crash under the same conditions and
> the only difference is the samplesPerFrame value. It seems clear that the
> asserts are unnecessarily causing a crash.


 a) asserts don't cause crashes, they cause signals which are not typically 
caught, and are not emitted in release builds
 b) those asserts were added because they were catching improper behavior by 
datasources and/or Kst
 c) "because it crashes the app" is not a reason why the assert is wrong.
Comment 7 Andrew Walker 2006-01-26 20:48:05 UTC
George, did you want to fix this or shall I?
Comment 8 George Staikos 2006-01-26 20:58:57 UTC
On Thursday 26 January 2006 14:48, Andrew Walker wrote:
> ------- George, did you want to fix this or shall I?


   I can look at it once the view object painting rework is done, reviewed, 
and committed.  Anyone else is free to submit a patch.  However I won't 
approve an unjustified patch that just removes the asserts and converts them 
to runtime checks.
Comment 9 Netterfield 2006-01-27 00:39:22 UTC
The situation is that there is no valid data in the sample range requested by the user.  What is the correct behavior?
  -read the requested number of points, but count from the end?
  -read nothing and leave the vector empty (can vectors/curves handle 0 length
   vectors?)
Comment 10 George Staikos 2006-01-27 01:02:36 UTC
On Thursday 26 January 2006 18:39, netterfield@astro.utoronto.ca wrote:

> 00:39 ------- The situation is that there is no valid data in the sample
> range requested by the user.  What is the correct behavior? -read the
> requested number of points, but count from the end?


  I don't think we should read something that the user didn't ask for.

>   -read nothing and leave the vector empty (can vectors/curves handle 0
> length vectors?)


  Exactly, this is [one of]the problem[s].  We cannot have 0 length vectors.  
Historical assumption is that length of vectors is always >= 1.  (Used to 
even be >= 2.)
Comment 11 George Staikos 2006-01-27 01:39:07 UTC
Around line 461 there is this:

    if (new_nf <= 0) {
      // Tried to read starting past the end.
      new_f0 = 0;
      new_nf = 1;
    }


   Is it being executed?  If so, maybe it needs to be more robust.  I think if 
we have a read-from-past end case, we need to, at the top, bail out and set 
the size to 0, filled with NAN.  It's a nonsense case.

   (and we should leave the code down lower as-is so we can catch errors)
Comment 12 Netterfield 2006-01-27 17:55:55 UTC
OK.  I think Andrew is right.  As the code was designed, the first assert is invalid: 
assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1);  should be       assert(new_nf - NF - 1 >=0 || new_nf - NF - 1 == -1);

or not be there at all.

What the code was suppose to do, on a start past eof, is to read only the first sample in the file.

Of course George is also right - this is not the most clever of all possible solutions.  In an attempt to be more clever, the patch below detects the start past eof condition, and makes a 1 sample long vector which contains only NaN.  I have not fully tested what happens in all cases when you try to make curves, equations, psds, etc, of vectors with only 1 sample, which is a NaN, but at first try, it seems ok (but so does deleting the assert).

Index: kstrvector.cpp
===================================================================
--- kstrvector.cpp      (revision 502941)
+++ kstrvector.cpp      (working copy)
@@ -430,7 +430,8 @@
   int i, k, shift, n_read=0;
   int ave_nread;
   int new_f0, new_nf;
-
+  bool start_past_eof = false;
+
   checkIntegrity();

   if (DoSkip && Skip < 2 && SPF == 1) {
@@ -462,6 +463,7 @@
       // Tried to read starting past the end.
       new_f0 = 0;
       new_nf = 1;
+      start_past_eof = true;
     }
   }

@@ -575,7 +577,10 @@
     }

     // read the new data from file
-    if (_file->samplesPerFrame(_field) > 1) {
+    if (start_past_eof) {
+      _v[0] = NAN;
+      n_read = 1;
+    } else if (_file->samplesPerFrame(_field) > 1) {
       assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1);
       assert(new_f0 + NF >= 0);
       assert(new_f0 + new_nf - 1 >= 0);
Comment 13 George Staikos 2006-01-27 22:09:59 UTC
This patch looks correct.  Reviewed=me.  Leaving the asserts there will help 
us catch regressions later.

On Friday 27 January 2006 11:55, netterfield@astro.utoronto.ca wrote:

> Index: kstrvector.cpp
> ===================================================================
> --- kstrvector.cpp      (revision 502941)
> +++ kstrvector.cpp      (working copy)
>  @ -430,7 +430,8  @
>    int i, k, shift, n_read=0;
>    int ave_nread;
>    int new_f0, new_nf;
> -
> +  bool start_past_eof = false;
> +
>    checkIntegrity();
>
>    if (DoSkip && Skip < 2 && SPF == 1) {
>  @ -462,6 +463,7  @
>        // Tried to read starting past the end.
>        new_f0 = 0;
>        new_nf = 1;
> +      start_past_eof = true;
>      }
>    }
>
>  @ -575,7 +577,10  @
>      }
>
>      // read the new data from file
> -    if (_file->samplesPerFrame(_field) > 1) {
> +    if (start_past_eof) {
> +      _v[0] = NAN;
> +      n_read = 1;
> +    } else if (_file->samplesPerFrame(_field) > 1) {
>        assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1);
>        assert(new_f0 + NF >= 0);
>        assert(new_f0 + new_nf - 1 >= 0);
> _______________________________________________
> Kst mailing list
> Kst@kde.org
> https://mail.kde.org/mailman/listinfo/kst

Comment 14 Netterfield 2006-01-28 01:36:48 UTC
SVN commit 503057 by netterfield:

BUG: 120823

Read past EOF results in a vector of 1 sample = NaN.



 M  +7 -2      kstrvector.cpp  


--- trunk/extragear/graphics/kst/kst/kstrvector.cpp #503056:503057
@@ -430,7 +430,8 @@
   int i, k, shift, n_read=0;
   int ave_nread;
   int new_f0, new_nf;
-
+  bool start_past_eof = false;
+  
   checkIntegrity();
 
   if (DoSkip && Skip < 2 && SPF == 1) {
@@ -462,6 +463,7 @@
       // Tried to read starting past the end.
       new_f0 = 0;
       new_nf = 1;
+      start_past_eof = true;
     }
   }
 
@@ -575,7 +577,10 @@
     }
 
     // read the new data from file
-    if (_file->samplesPerFrame(_field) > 1) {
+    if (start_past_eof) {
+      _v[0] = NAN;
+      n_read = 1;
+    } else if (_file->samplesPerFrame(_field) > 1) {
       assert(new_nf - NF - 1 > 0 || new_nf - NF - 1 == -1);
       assert(new_f0 + NF >= 0);
       assert(new_f0 + new_nf - 1 >= 0);