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.

Sign up for GitHub

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

Rostime overflow bugs #61

Merged
merged 2 commits into from
Jul 21, 2017
Merged

Rostime overflow bugs #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf5175a
add tests for overflow bugs in Time and Duration from #58
subject-name-here May 6, 2017
8ad6863
make new tests for overflow bugs in Time and Duration pass
dirk-thomas Jul 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions rostime/include/ros/impl/duration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,26 +52,30 @@ namespace ros {
template<class T>
T& DurationBase<T>::fromSec(double d)
{
sec = (int32_t)floor(d);
int64_t sec64 = (int64_t)floor(d);
if (sec64 < INT_MIN || sec64 > INT_MAX)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might consider using INT32_MIN and INT32_MAX instead, reference:

http://en.cppreference.com/w/cpp/types/integer

(here and below)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree using (U)INT32_MIN|MAX would indeed be nicer. I kept it with (U)INT_MIN|MAX for now since those are already being used within the code. I would rather not mix them and don't want to update the existing code.

throw std::runtime_error("Duration is out of dual 32-bit range");
sec = (int32_t)sec64;
nsec = (int32_t)((d - (double)sec)*1000000000);
return *static_cast<T*>(this);
}

template<class T>
T& DurationBase<T>::fromNSec(int64_t t)
{
sec = (int32_t)(t / 1000000000);
int64_t sec64 = t / 1000000000;
if (sec64 < INT_MIN || sec64 > INT_MAX)
throw std::runtime_error("Duration is out of dual 32-bit range");
sec = (int32_t)sec64;
nsec = (int32_t)(t % 1000000000);

normalizeSecNSecSigned(sec, nsec);

return *static_cast<T*>(this);
}

template<class T>
T DurationBase<T>::operator+(const T &rhs) const
{
return T(sec + rhs.sec, nsec + rhs.nsec);
T t;
return t.fromNSec(toNSec() + rhs.toNSec());
}

template<class T>
Expand All @@ -83,13 +87,15 @@ namespace ros {
template<class T>
T DurationBase<T>::operator-(const T &rhs) const
{
return T(sec - rhs.sec, nsec - rhs.nsec);
T t;
return t.fromNSec(toNSec() - rhs.toNSec());
}

template<class T>
T DurationBase<T>::operator-() const
{
return T(-sec , -nsec);
T t;
return t.fromNSec(-toNSec());
}

template<class T>
Expand Down
4 changes: 2 additions & 2 deletions rostime/include/ros/impl/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ namespace ros
template<class T, class D>
D TimeBase<T, D>::operator-(const T &rhs) const
{
return D((int32_t)sec - (int32_t)rhs.sec,
(int32_t)nsec - (int32_t)rhs.nsec); // carry handled in ctor
D d;
return d.fromNSec(toNSec() - rhs.toNSec());
}

template<class T, class D>
Expand Down
5 changes: 4 additions & 1 deletion rostime/include/ros/time.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ namespace ros

double toSec() const { return (double)sec + 1e-9*(double)nsec; };
T& fromSec(double t) {
sec = (uint32_t)floor(t);
int64_t sec64 = (int64_t)floor(t);
if (sec64 < 0 || sec64 > UINT_MAX)
throw std::runtime_error("Duration is out of dual 32-bit range");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider changing this to:
"Time is out of dual unsigned 32-bit range".
Avoids ambiguity for people who are trying to debug, and it is more accurate. The ros_comm repo is currently failing the empty_timestamp_crash_check test with this exception, which is why I'm making the suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing it out. Fixed in f824e46.

sec = (int32_t)sec64;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cast to int32_t, not to uint32_t? For me, uint seems logical.

Copy link
Member Author

@dirk-thomas dirk-thomas Jul 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. Thank you for catching this. Too much copy-n-paste between Duration and Time. I created #63 to address it.

nsec = (uint32_t)boost::math::round((t-sec) * 1e9);
// avoid rounding errors
sec += (nsec / 1000000000ul);
Expand Down
80 changes: 80 additions & 0 deletions rostime/test/duration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,86 @@ TEST(Duration, sleepWithSimTime)
ASSERT_FALSE(rc);
}

TEST(Duration, castFromDoubleExceptions)
{
ros::Time::init();

Duration d1, d2, d3, d4;
// Valid values to cast, must not throw exceptions
EXPECT_NO_THROW(d1.fromSec(-2147483648.0));
EXPECT_NO_THROW(d2.fromSec(-2147483647.999999));
EXPECT_NO_THROW(d3.fromSec(2147483647.0));
EXPECT_NO_THROW(d4.fromSec(2147483647.999999));

// The next casts all incorrect.
EXPECT_THROW(d1.fromSec(2147483648.0), std::runtime_error);
EXPECT_THROW(d2.fromSec(6442450943.0), std::runtime_error); // It's 2^31 - 1 + 2^32, and it could pass the test.
EXPECT_THROW(d3.fromSec(-2147483648.001), std::runtime_error);
EXPECT_THROW(d4.fromSec(-6442450943.0), std::runtime_error);
}

TEST(Duration, castFromInt64Exceptions)
{
ros::Time::init();

Duration d1, d2, d3, d4;
// Valid values to cast, must not throw exceptions
EXPECT_NO_THROW(d1.fromNSec(-2147483648000000000));
EXPECT_NO_THROW(d2.fromNSec(-2147483647999999999));
EXPECT_NO_THROW(d3.fromNSec(2147483647000000000));
EXPECT_NO_THROW(d4.fromNSec(2147483647999999999));

// The next casts all incorrect.
EXPECT_THROW(d1.fromSec(2147483648000000000), std::runtime_error);
EXPECT_THROW(d2.fromSec(4294967296000000000), std::runtime_error);
EXPECT_THROW(d3.fromSec(-2147483648000000001), std::runtime_error);
EXPECT_THROW(d4.fromSec(-6442450943000000000), std::runtime_error);
}

TEST(Duration, arithmeticExceptions)
{
ros::Time::init();

Duration d1(2147483647, 0);
Duration d2(2147483647, 999999999);
EXPECT_THROW(d1 + d2, std::runtime_error);

Duration d3(-2147483648, 0);
Duration d4(2147483647, 0);
EXPECT_THROW(d3 - d4, std::runtime_error);
EXPECT_THROW(d4 - d3, std::runtime_error);

Duration d5(-2147483647, 1);
Duration d6(-2, 999999999);
Duration d7;
EXPECT_NO_THROW(d7 = d5 + d6);
EXPECT_EQ(-2147483648000000000, d7.toNSec());
}

TEST(Duration, negativeSignExceptions)
{
ros::Time::init();

Duration d1(2147483647, 0);
Duration d2(2147483647, 999999999);
Duration d3;
EXPECT_NO_THROW(d3 = -d1);
EXPECT_EQ(-2147483647000000000, d3.toNSec());
EXPECT_NO_THROW(d3 = -d2);
EXPECT_EQ(-2147483647999999999, d3.toNSec());

Duration d4(-2147483647, 0);
Duration d5(-2147483648, 999999999);
Duration d6(-2147483648, 2);
Duration d7;
EXPECT_NO_THROW(d7 = -d4);
EXPECT_EQ(2147483647000000000, d7.toNSec());
EXPECT_NO_THROW(d7 = -d5);
EXPECT_EQ(2147483647000000001, d7.toNSec());
EXPECT_NO_THROW(d7 = -d6);
EXPECT_EQ(2147483647999999998, d7.toNSec());
}

int main(int argc, char **argv){
testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
Expand Down
70 changes: 70 additions & 0 deletions rostime/test/time.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,76 @@ TEST(Time, ToFromBoost)
}
}

TEST(Time, CastFromDoubleExceptions)
{
ros::Time::init();

Time t1, t2, t3;
// Valid values to cast, must not throw exceptions
EXPECT_NO_THROW(t1.fromSec(4294967295.0));
EXPECT_NO_THROW(t2.fromSec(4294967295.999));
EXPECT_NO_THROW(t3.fromSec(0.0000001));

// The next casts all incorrect.
EXPECT_THROW(t1.fromSec(4294967296.0), std::runtime_error);
EXPECT_THROW(t2.fromSec(-0.0001), std::runtime_error);
EXPECT_THROW(t3.fromSec(-4294967296.0), std::runtime_error);
}

TEST(Time, OperatorMinusExceptions)
{
ros::Time::init();

Time t1(2147483648, 0);
Time t2(2147483647, 999999999);
Time t3(2147483647, 999999998);
Time t4(4294967295, 999999999);
Time t5(4294967295, 999999998);
Time t6(0, 1);

Duration d1(2147483647, 999999999);
Duration d2(-2147483648, 0);
Duration d3(-2147483648, 1);
Duration d4(0, 1);

EXPECT_NO_THROW(t1 - t2);
EXPECT_NO_THROW(t3 - t2);
EXPECT_NO_THROW(t4 - t5);

EXPECT_NO_THROW(t1 - d1);
EXPECT_NO_THROW(t5 - d1);

EXPECT_THROW(t4 - t6, std::runtime_error);
EXPECT_THROW(t4 - t3, std::runtime_error);

EXPECT_THROW(t1 - d2, std::runtime_error);
EXPECT_THROW(t2 - d2, std::runtime_error);
EXPECT_THROW(t4 - d3, std::runtime_error);
}

TEST(Time, OperatorPlusExceptions)
{
ros::Time::init();

Time t1(2147483648, 0);
Time t2(2147483647, 999999999);
Time t4(4294967295, 999999999);
Time t5(4294967295, 999999998);

Duration d1(2147483647, 999999999);
Duration d2(-2147483648, 1);
Duration d3(0, 2);
Duration d4(0, 1);

EXPECT_NO_THROW(t2 + d2);
EXPECT_NO_THROW(t1 + d1);

EXPECT_THROW(t4 + d4, std::runtime_error);
EXPECT_THROW(t4 + d1, std::runtime_error);
EXPECT_THROW(t5 + d3, std::runtime_error);
}


/************************************* Duration Tests *****************/

TEST(Duration, Comparitors)
Expand Down