View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0001532OpenFOAMBugpublic2015-02-16 13:562015-02-18 18:21
Reporteruser528Assigned Tohenry  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
PlatformLinuxOSSUSE Linux Enterprise ServerOS Version11.2
Summary0001532: readInt does not check for overflow
DescriptionreadInt(const char*, int&) does not check for overflow nor in strtol(), nor while converting long to int. As a result, the resulting int value and the return bool value can be silently wrong.
Steps To ReproduceCompile and run the following program:

#include "fvCFD.H"

int main(int argc, char *argv[])
{
    std::string s("12345678901234");
    int a;
    bool res = readInt(s.c_str(), a);
    Info << a << " " << res <<endl;

    IStringStream str("12345678901234");
    scalar x = readScalar(str);
    Info << x << endl;

    return 0;
}

It outputs
-1 1
1.94289e+09
while it should output something like
-1 0
1.23457e+13

The difference is the return (bool) value of readInt and the return value of readScalar.
Additional InformationI have ran into this problem in the following way. I have one OpenFOAM code that generates some dictionary, and another OpenFOAM code that reads this dictionary. Among others, there is a scalar value in this dictionary, that in one of my tests was equal to about 3.4e11.

I have writePrecision 12 in my controlDict, so that scalar value was printed just like 340853124000 --- no decimal dot, no "e" sign. When I was reading it back, the ISstream::read(token& t) sees that there are only digits in the token, so it tries to call readLabel. The latter returns true despite the fact that the value overflows.

A diff for a fix is attached.
TagsNo tags attached.

Activities

user528

2015-02-16 13:56

 

readInt.diff (713 bytes)    
diff --git a/src/OpenFOAM/primitives/ints/int/intIO.C b/src/OpenFOAM/primitives/ints/int/intIO.C
index bd02519..74243d4 100644
--- a/src/OpenFOAM/primitives/ints/int/intIO.C
+++ b/src/OpenFOAM/primitives/ints/int/intIO.C
@@ -35,6 +35,7 @@ Description
 #include "IOstreams.H"
 
 #include <sstream>
+#include <cerrno>
 
 // * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * //
 
@@ -91,9 +92,10 @@ int Foam::readInt(Istream& is)
 bool Foam::readInt(const char* buf, int& s)
 {
     char *endptr = NULL;
+    errno = 0;
     long l = strtol(buf, &endptr, 10);
     s = int(l);
-    return (*endptr == 0);
+    return (*endptr == 0) && (errno == 0) && (l >= INT_MIN) && (l <= INT_MAX);
 }
readInt.diff (713 bytes)   

henry

2015-02-16 22:03

manager   ~0003806

Thanks for the report and fix.
Resolved by commit 57407a029c02d290fc6fcce39e0db99c6fc50ac5

user4

2015-02-18 17:41

 

cavity.tgz (6,165 bytes)

user4

2015-02-18 17:44

  ~0003831

The cast from long to int32_t in intIO.C can still overflow. Attached a small case that demonstrates it. Run icoFoam and check 0.005/U - the uniformFixedValue table will be incorrect.

henry

2015-02-18 17:48

manager   ~0003832

Would you like the patch removed?

user4

2015-02-18 17:56

  ~0003833

No, the patch is ok but only checks the result of strtol. In addition before you do the

    s = int32_t(l);

you should first check for l<INT32_MIN or l>INT32_MAX (unless there is a better way of detecting overflow)

henry

2015-02-18 18:08

manager   ~0003834

This assumes that int32 != long int but it might. For int64 should we use strtol or strtoll?

       long int strtol(const char *nptr, char **endptr, int base);

       long long int strtoll(const char *nptr, char **endptr, int base);

how do we know the correspondence between these functions and the int32 and int64 types? Another option would be to use strtoimax, what do you think?

user4

2015-02-18 18:12

  ~0003835

Didn't know about strtoimax. Looks a better fit.

henry

2015-02-18 18:21

manager   ~0003836

OK, I will make this change. I am assuming that intmax_t is not larger than int64_t and hence no need to test against INT64_MIN/MAX.

Issue History

Date Modified Username Field Change
2015-02-16 13:56 user528 New Issue
2015-02-16 13:56 user528 File Added: readInt.diff
2015-02-16 22:03 henry Note Added: 0003806
2015-02-16 22:03 henry Status new => resolved
2015-02-16 22:03 henry Resolution open => fixed
2015-02-16 22:03 henry Assigned To => henry
2015-02-18 17:41 user4 File Added: cavity.tgz
2015-02-18 17:44 user4 Note Added: 0003831
2015-02-18 17:48 henry Note Added: 0003832
2015-02-18 17:56 user4 Note Added: 0003833
2015-02-18 18:08 henry Note Added: 0003834
2015-02-18 18:12 user4 Note Added: 0003835
2015-02-18 18:21 henry Note Added: 0003836