Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

.NET Binding: Setting keepAliveTimeout to TimeSpan.Max will flood connection #381

Closed
sidhoda opened this issue May 29, 2020 · 6 comments
Closed

Comments

@sidhoda
Copy link
Contributor

sidhoda commented May 29, 2020

TimeSpan.Max is converted to milliseconds as 922337203685477, which is passed into TimeDuration ctor as a duration_t value of -580800 nanoseconds. This causes the keep alive timer to constantly fire, flooding the connection.

The following fix works, however I'm sure there's a better way.

opendnp3::TimeDuration Conversions::ConvertTimespan(System::TimeSpan ts)
{
    if (ts == System::TimeSpan::MaxValue)
        return opendnp3::TimeDuration::Max();

    return ConvertMilliseconds(ts.Ticks / System::TimeSpan::TicksPerMillisecond);
}
@sidhoda
Copy link
Contributor Author

sidhoda commented May 29, 2020

Just to add this is on x86 build, in case that matters.

@jadamcrain
Copy link
Member

Is this a regression in3.0 or also an issue on 2.x?

@sidhoda
Copy link
Contributor Author

sidhoda commented May 29, 2020

We believe it was a regression in 3.0.
There was a previous similar bug #175 fixed in 2.x

@jadamcrain
Copy link
Member

jadamcrain commented Jun 1, 2020

The underlying issue is that we switched the duration representation in 3.0 to use std::chrono::duration which overflows here only on 32-bit platforms.

We need to clamp the values to their max/min representations here.

std::chrono is kind of a dumpster fire with all the special rules and different bit widths for different templates specializations :/

https://en.cppreference.com/w/cpp/chrono/duration

@jadamcrain
Copy link
Member

@sidhoda Can you continue your evaluation on the develop branch? I've just merged a PR to handle the overflow, but want to be sure in our environment before doing a release

@dwiley007
Copy link
Contributor

@jadamcrain I've tested the change on behalf of @sidhoda and it no longer floods the connection.

@emgre emgre closed this as completed Jun 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants