Re: [PATCH lttng-tools] Fix: nsec diff can be negative leading to expeditive connection timeout

Re: [PATCH lttng-tools] Fix: nsec diff can be negative leading to expeditive connection timeout

[Simon Marchi]

On 2018-11-08 17:08, Jonathan Rajotte wrote:
> The nanoseconds part of the timespec struct time_a is not always
> bigger than time_b since it wrap around each seconds.
> 
> Use the absolute value of the nanosecond difference to perform
> unsigned long operation.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/common/sessiond-comm/inet.c  | 2 +-
>  src/common/sessiond-comm/inet6.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/common/sessiond-comm/inet.c 
> b/src/common/sessiond-comm/inet.c
> index e0b3e7a96..cb6f45357 100644
> --- a/src/common/sessiond-comm/inet.c
> +++ b/src/common/sessiond-comm/inet.c
> @@ -124,7 +124,7 @@ unsigned long time_diff_ms(struct timespec *time_a,
>  	unsigned long result_ms;
> 
>  	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
> 
>  	result_ms = sec_diff * MSEC_PER_SEC;
>  	result_ms += nsec_diff / NSEC_PER_MSEC;
> diff --git a/src/common/sessiond-comm/inet6.c 
> b/src/common/sessiond-comm/inet6.c
> index dfb5fc5d1..b73802d48 100644
> --- a/src/common/sessiond-comm/inet6.c
> +++ b/src/common/sessiond-comm/inet6.c
> @@ -122,7 +122,7 @@ unsigned long time_diff_ms(struct timespec *time_a,
>  	unsigned long result_ms;
> 
>  	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
> 
>  	result_ms = sec_diff * MSEC_PER_SEC;
>  	result_ms += nsec_diff / NSEC_PER_MSEC;

That doesn't look right either.  With

time_a = 2.9 s
time_b = 3.1 s

it will result in

sec_diff = 1 s
nsec_diff = 0.8 s

Added, the result will be 1.8 seconds, when actually we expect 0.2 s.  
You probably have no choice but do the "borrowing" by hand, like we did 
when learning subtraction in elementary school :).  Maybe this deserves 
some unit tests, since it's apparently easy to get it wrong.

Simon

Re: [PATCH lttng-tools] Fix: nsec diff can be negative leading to expeditive connection timeout

[Simon Marchi]

On 2018-11-08 17:17, Simon Marchi wrote:
> On 2018-11-08 17:08, Jonathan Rajotte wrote:
>> The nanoseconds part of the timespec struct time_a is not always
>> bigger than time_b since it wrap around each seconds.
>> 
>> Use the absolute value of the nanosecond difference to perform
>> unsigned long operation.
>> 
>> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
>> ---
>>  src/common/sessiond-comm/inet.c  | 2 +-
>>  src/common/sessiond-comm/inet6.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/common/sessiond-comm/inet.c 
>> b/src/common/sessiond-comm/inet.c
>> index e0b3e7a96..cb6f45357 100644
>> --- a/src/common/sessiond-comm/inet.c
>> +++ b/src/common/sessiond-comm/inet.c
>> @@ -124,7 +124,7 @@ unsigned long time_diff_ms(struct timespec 
>> *time_a,
>>  	unsigned long result_ms;
>> 
>>  	sec_diff = time_a->tv_sec - time_b->tv_sec;
>> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
>> +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
>> 
>>  	result_ms = sec_diff * MSEC_PER_SEC;
>>  	result_ms += nsec_diff / NSEC_PER_MSEC;
>> diff --git a/src/common/sessiond-comm/inet6.c 
>> b/src/common/sessiond-comm/inet6.c
>> index dfb5fc5d1..b73802d48 100644
>> --- a/src/common/sessiond-comm/inet6.c
>> +++ b/src/common/sessiond-comm/inet6.c
>> @@ -122,7 +122,7 @@ unsigned long time_diff_ms(struct timespec 
>> *time_a,
>>  	unsigned long result_ms;
>> 
>>  	sec_diff = time_a->tv_sec - time_b->tv_sec;
>> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
>> +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
>> 
>>  	result_ms = sec_diff * MSEC_PER_SEC;
>>  	result_ms += nsec_diff / NSEC_PER_MSEC;
> 
> That doesn't look right either.  With
> 
> time_a = 2.9 s
> time_b = 3.1 s
> 
> it will result in
> 
> sec_diff = 1 s
> nsec_diff = 0.8 s
> 
> Added, the result will be 1.8 seconds, when actually we expect 0.2 s.
> You probably have no choice but do the "borrowing" by hand, like we
> did when learning subtraction in elementary school :).  Maybe this
> deserves some unit tests, since it's apparently easy to get it wrong.

Actually, see here for an example:

https://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html

Simon

Re: [PATCH lttng-tools] Fix: nsec diff can be negative leading to expeditive connection timeout

[Jonathan Rajotte-Julien]

You are right...Might have been looking to long at logs all day.

Thanks for pointing it out.

Will send v2 shortly.

On Thu, Nov 08, 2018 at 05:26:39PM -0500, Simon Marchi wrote:
> On 2018-11-08 17:17, Simon Marchi wrote:
> > On 2018-11-08 17:08, Jonathan Rajotte wrote:
> > > The nanoseconds part of the timespec struct time_a is not always
> > > bigger than time_b since it wrap around each seconds.
> > > 
> > > Use the absolute value of the nanosecond difference to perform
> > > unsigned long operation.
> > > 
> > > Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> > > ---
> > >  src/common/sessiond-comm/inet.c  | 2 +-
> > >  src/common/sessiond-comm/inet6.c | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/common/sessiond-comm/inet.c
> > > b/src/common/sessiond-comm/inet.c
> > > index e0b3e7a96..cb6f45357 100644
> > > --- a/src/common/sessiond-comm/inet.c
> > > +++ b/src/common/sessiond-comm/inet.c
> > > @@ -124,7 +124,7 @@ unsigned long time_diff_ms(struct timespec
> > > *time_a,
> > >  	unsigned long result_ms;
> > > 
> > >  	sec_diff = time_a->tv_sec - time_b->tv_sec;
> > > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> > > +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
> > > 
> > >  	result_ms = sec_diff * MSEC_PER_SEC;
> > >  	result_ms += nsec_diff / NSEC_PER_MSEC;
> > > diff --git a/src/common/sessiond-comm/inet6.c
> > > b/src/common/sessiond-comm/inet6.c
> > > index dfb5fc5d1..b73802d48 100644
> > > --- a/src/common/sessiond-comm/inet6.c
> > > +++ b/src/common/sessiond-comm/inet6.c
> > > @@ -122,7 +122,7 @@ unsigned long time_diff_ms(struct timespec
> > > *time_a,
> > >  	unsigned long result_ms;
> > > 
> > >  	sec_diff = time_a->tv_sec - time_b->tv_sec;
> > > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> > > +	nsec_diff = labs(time_a->tv_nsec - time_b->tv_nsec);
> > > 
> > >  	result_ms = sec_diff * MSEC_PER_SEC;
> > >  	result_ms += nsec_diff / NSEC_PER_MSEC;
> > 
> > That doesn't look right either.  With
> > 
> > time_a = 2.9 s
> > time_b = 3.1 s
> > 
> > it will result in
> > 
> > sec_diff = 1 s
> > nsec_diff = 0.8 s
> > 
> > Added, the result will be 1.8 seconds, when actually we expect 0.2 s.
> > You probably have no choice but do the "borrowing" by hand, like we
> > did when learning subtraction in elementary school :).  Maybe this
> > deserves some unit tests, since it's apparently easy to get it wrong.
> 
> Actually, see here for an example:
> 
> https://www.gnu.org/software/libc/manual/html_node/Elapsed-Time.html
> 
> Simon

-- 
Jonathan Rajotte-Julien
EfficiOS

[PATCH lttng-tools v2] Fix: timeout for connect is not respected

[Jonathan Rajotte]

Problem:

The previous algorithm was not *wrong* but was prone to C type conversion
error.

Previous algorithm:

((A.sec - B.sec) * MSEC_PER_SEC) + ((A.nsec - B.nsec) / NSEC_PER_SEC)

((A.sec - B.sec) * 1000000ULL) + ((A.nsec - B.nsec) / 1000000ULL)

Note that "(A.nsec - B.nsec)" can be negative.

A *quick* fix here could be to cast the NSEC_PER_SEC to long allowing a
valid division operation.

The problem was introduced by 395d6b02dda3db1acd08936f49c1dc8efc48e613.

Solution:

Perform the time diff using a temporary timespec struct and perform the
carry operation manually. This prevent negative value for nsec and 
helps prevent C type promotion problems.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---

I initially misplaced the issue.
As mentioned in the commit message, we can also cast NSEC_PER_SEC to long to fix
the issue. I prefer the use of a timespec struct for expressiveness.

Also the case were we pass a time_b > time_a is handled by returning a
difference of zero. We might want to assert here since it should never happen.

 src/common/sessiond-comm/inet.c  | 28 ++++++++++++++++++++--------
 src/common/sessiond-comm/inet6.c | 28 ++++++++++++++++++++--------
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
index e0b3e7a96..94a3c3a67 100644
--- a/src/common/sessiond-comm/inet.c
+++ b/src/common/sessiond-comm/inet.c
@@ -119,16 +119,28 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	if (diff.tv_sec < 0) {
+		/* We went back in time. Put all to zero */
+		diff.tv_sec = 0;
+		diff.tv_nsec = 0;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tb_sec is always positive.
+	 */
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
index dfb5fc5d1..07043e93f 100644
--- a/src/common/sessiond-comm/inet6.c
+++ b/src/common/sessiond-comm/inet6.c
@@ -117,16 +117,28 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	if (diff.tv_sec < 0) {
+		/* We went back in time. Put all to zero */
+		diff.tv_sec = 0;
+		diff.tv_nsec = 0;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tb_sec is always positive.
+	 */
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
-- 
2.17.1

Re: [PATCH lttng-tools v2] Fix: timeout for connect is not respected

[Mathieu Desnoyers]

----- On Nov 9, 2018, at 10:37 AM, Jonathan Rajotte jonathan.rajotte-julien@efficios.com wrote:

> Problem:
> 
> The previous algorithm was not *wrong* but was prone to C type conversion
> error.
> 
> Previous algorithm:
> 
> ((A.sec - B.sec) * MSEC_PER_SEC) + ((A.nsec - B.nsec) / NSEC_PER_SEC)
> 
> ((A.sec - B.sec) * 1000000ULL) + ((A.nsec - B.nsec) / 1000000ULL)
> 
> Note that "(A.nsec - B.nsec)" can be negative.
> 
> A *quick* fix here could be to cast the NSEC_PER_SEC to long allowing a
> valid division operation.
> 
> The problem was introduced by 395d6b02dda3db1acd08936f49c1dc8efc48e613.
> 
> Solution:
> 
> Perform the time diff using a temporary timespec struct and perform the
> carry operation manually. This prevent negative value for nsec and
> helps prevent C type promotion problems.
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
> 
> I initially misplaced the issue.
> As mentioned in the commit message, we can also cast NSEC_PER_SEC to long to fix
> the issue. I prefer the use of a timespec struct for expressiveness.
> 
> Also the case were we pass a time_b > time_a is handled by returning a
> difference of zero. We might want to assert here since it should never happen.
> 
> src/common/sessiond-comm/inet.c  | 28 ++++++++++++++++++++--------
> src/common/sessiond-comm/inet6.c | 28 ++++++++++++++++++++--------
> 2 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
> index e0b3e7a96..94a3c3a67 100644
> --- a/src/common/sessiond-comm/inet.c
> +++ b/src/common/sessiond-comm/inet.c
> @@ -119,16 +119,28 @@ static
> unsigned long time_diff_ms(struct timespec *time_a,
> 		struct timespec *time_b)
> {
> -	time_t sec_diff;
> -	long nsec_diff;
> -	unsigned long result_ms;
> +	struct timespec diff;
> 
> -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
> +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
> 
> -	result_ms = sec_diff * MSEC_PER_SEC;
> -	result_ms += nsec_diff / NSEC_PER_MSEC;
> -	return result_ms;
> +	if (diff.tv_nsec < 0) {
> +		/* Perform borrow operation */
> +		diff.tv_sec -= 1;
> +		diff.tv_nsec += NSEC_PER_SEC;
> +	}
> +
> +	if (diff.tv_sec < 0) {
> +		/* We went back in time. Put all to zero */
> +		diff.tv_sec = 0;
> +		diff.tv_nsec = 0;
> +	}

Why would a diff operation implicitly floor everything to 0 when
time_a < time_b ?

> +
> +	/*
> +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
> +	 * operation. diff.tb_sec is always positive.

tb_sec -> tv_nsec


> +	 */
> +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);

How do we handle time delta difference that overflows unsigned long ?

> }
> 
> static
> diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
> index dfb5fc5d1..07043e93f 100644
> --- a/src/common/sessiond-comm/inet6.c
> +++ b/src/common/sessiond-comm/inet6.c
> @@ -117,16 +117,28 @@ static
> unsigned long time_diff_ms(struct timespec *time_a,
> 		struct timespec *time_b)

This should be moved to a common implementation file. It's a cut and
paste from inet.c.

Thanks,

Mathieu

> {
> -	time_t sec_diff;
> -	long nsec_diff;
> -	unsigned long result_ms;
> +	struct timespec diff;
> 
> -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
> +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
> 
> -	result_ms = sec_diff * MSEC_PER_SEC;
> -	result_ms += nsec_diff / NSEC_PER_MSEC;
> -	return result_ms;
> +	if (diff.tv_nsec < 0) {
> +		/* Perform borrow operation */
> +		diff.tv_sec -= 1;
> +		diff.tv_nsec += NSEC_PER_SEC;
> +	}
> +
> +	if (diff.tv_sec < 0) {
> +		/* We went back in time. Put all to zero */
> +		diff.tv_sec = 0;
> +		diff.tv_nsec = 0;
> +	}
> +
> +	/*
> +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
> +	 * operation. diff.tb_sec is always positive.
> +	 */
> +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
> }
> 
> static
> --
> 2.17.1
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH lttng-tools v2] Fix: timeout for connect is not respected

[Jonathan Rajotte-Julien]

> > diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
> > index e0b3e7a96..94a3c3a67 100644
> > --- a/src/common/sessiond-comm/inet.c
> > +++ b/src/common/sessiond-comm/inet.c
> > @@ -119,16 +119,28 @@ static
> > unsigned long time_diff_ms(struct timespec *time_a,
> > 		struct timespec *time_b)
> > {
> > -	time_t sec_diff;
> > -	long nsec_diff;
> > -	unsigned long result_ms;
> > +	struct timespec diff;
> > 
> > -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> > +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
> > +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
> > 
> > -	result_ms = sec_diff * MSEC_PER_SEC;
> > -	result_ms += nsec_diff / NSEC_PER_MSEC;
> > -	return result_ms;
> > +	if (diff.tv_nsec < 0) {
> > +		/* Perform borrow operation */
> > +		diff.tv_sec -= 1;
> > +		diff.tv_nsec += NSEC_PER_SEC;
> > +	}
> > +
> > +	if (diff.tv_sec < 0) {
> > +		/* We went back in time. Put all to zero */
> > +		diff.tv_sec = 0;
> > +		diff.tv_nsec = 0;
> > +	}
> 
> Why would a diff operation implicitly floor everything to 0 when
> time_a < time_b ?

Sure I can implement it as a real difference without the same "assumptions" of
time_a > time_b since we are comparing passing time.

Make sure to tell this to past Mathieu ;).

> 
> > +
> > +	/*
> > +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
> > +	 * operation. diff.tb_sec is always positive.
> 
> tb_sec -> tv_nsec

Fixed.

> 
> 
> > +	 */
> > +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
> 
> How do we handle time delta difference that overflows unsigned long ?

Let's do a quick calculation. Correct me if anything is wrong here.

How many msec does a unsigned long hold?
A long is *minimum* 32 bits. Yielding a maximum value of 4294967295. [1]

Lets assume that the nanoseconds part of the diff struct is maxed out. The
nanoseconds part yield 99 ms.

4294967295 - 99 = 4294967196ms

How big would the second field of diff be a problem?

4294967196/ MSEC_PER_SEC = 4294967s or 49 days.

I would err on the side of "that it's enough". We also have to take into
account that for this to happen either something is going horribly wrong during
getting the current time or the user purposely set the timeout to this value
since we sample every 200ms.
Which at this point goes into the PEBKAC category.

Do you agree?

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf p.22 §5.2.4.2.1

> 
> > }
> > 
> > static
> > diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
> > index dfb5fc5d1..07043e93f 100644
> > --- a/src/common/sessiond-comm/inet6.c
> > +++ b/src/common/sessiond-comm/inet6.c
> > @@ -117,16 +117,28 @@ static
> > unsigned long time_diff_ms(struct timespec *time_a,
> > 		struct timespec *time_b)
> 
> This should be moved to a common implementation file. It's a cut and
> paste from inet.c.

Past Mathieu did not seems to bother here.

I'm open into doing this in two patches since this should be backported. Do you
have a suggestion regarding where to put it?

> 
> Thanks,
> 
> Mathieu
> 
> > {
> > -	time_t sec_diff;
> > -	long nsec_diff;
> > -	unsigned long result_ms;
> > +	struct timespec diff;
> > 
> > -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
> > +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
> > +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
> > 
> > -	result_ms = sec_diff * MSEC_PER_SEC;
> > -	result_ms += nsec_diff / NSEC_PER_MSEC;
> > -	return result_ms;
> > +	if (diff.tv_nsec < 0) {
> > +		/* Perform borrow operation */
> > +		diff.tv_sec -= 1;
> > +		diff.tv_nsec += NSEC_PER_SEC;
> > +	}
> > +
> > +	if (diff.tv_sec < 0) {
> > +		/* We went back in time. Put all to zero */
> > +		diff.tv_sec = 0;
> > +		diff.tv_nsec = 0;
> > +	}
> > +
> > +	/*
> > +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
> > +	 * operation. diff.tb_sec is always positive.
> > +	 */
> > +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
> > }
> > 
> > static
> > --
> > 2.17.1
> > 
> > _______________________________________________
> > lttng-dev mailing list
> > lttng-dev@lists.lttng.org
> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

-- 
Jonathan Rajotte-Julien
EfficiOS

[PATCH lttng-tools v3] Fix: timeout for connect is not respected

[Jonathan Rajotte]

Problem:

The previous algorithm was not *wrong* but was prone to C type conversion
error.

Previous algorithm:

((A.sec - B.sec) * MSEC_PER_SEC) + ((A.nsec - B.nsec) / NSEC_PER_SEC)

((A.sec - B.sec) * 1000000ULL) + ((A.nsec - B.nsec) / 1000000ULL)

Note that "(A.nsec - B.nsec)" can be negative.

A *quick* fix here is to cast the NSEC_PER_SEC to long allowing a valid
division operation.

The problem was introduced by 395d6b02dda3db1acd08936f49c1dc8efc48e613.

Solution:

Perform the time diff using a temporary timespec struct and perform the
carry operation manually. This prevent negative value for nsec and help
prevent C type promotion problems.

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---

Implement difference without posing the hypothesis that time_a is necessarily
bigger than time_b.

Fixed type in comment.

Added an assert statement to ensure premises before returning. Feel free to
remove.

 src/common/sessiond-comm/inet.c  | 32 ++++++++++++++++++++++++--------
 src/common/sessiond-comm/inet6.c | 32 ++++++++++++++++++++++++--------
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
index e0b3e7a96..f03fd1fb5 100644
--- a/src/common/sessiond-comm/inet.c
+++ b/src/common/sessiond-comm/inet.c
@@ -119,16 +119,32 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	if ((time_a->tv_sec > time_b->tv_sec)
+		|| ((time_a->tv_sec == time_b->tv_sec)
+			&& (time_a->tv_nsec > time_b->tv_nsec))) {
+		/* time_a is bigger */
+		diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+		diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
+	} else {
+		/* time_b is bigger */
+		diff.tv_sec = time_b->tv_sec - time_a->tv_sec;
+		diff.tv_nsec = time_b->tv_nsec - time_a->tv_nsec;
+	}
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tv_sec is always positive.
+	 */
+	assert(diff.tv_sec >= 0 && diff.tv_nsec >= 0);
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
index dfb5fc5d1..beb2dd9ed 100644
--- a/src/common/sessiond-comm/inet6.c
+++ b/src/common/sessiond-comm/inet6.c
@@ -117,16 +117,32 @@ static
 unsigned long time_diff_ms(struct timespec *time_a,
 		struct timespec *time_b)
 {
-	time_t sec_diff;
-	long nsec_diff;
-	unsigned long result_ms;
+	struct timespec diff;
 
-	sec_diff = time_a->tv_sec - time_b->tv_sec;
-	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
+	if ((time_a->tv_sec > time_b->tv_sec)
+		|| ((time_a->tv_sec == time_b->tv_sec)
+			&& (time_a->tv_nsec > time_b->tv_nsec))) {
+		/* time_a is bigger */
+		diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
+		diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
+	} else {
+		/* time_b is bigger */
+		diff.tv_sec = time_b->tv_sec - time_a->tv_sec;
+		diff.tv_nsec = time_b->tv_nsec - time_a->tv_nsec;
+	}
 
-	result_ms = sec_diff * MSEC_PER_SEC;
-	result_ms += nsec_diff / NSEC_PER_MSEC;
-	return result_ms;
+	if (diff.tv_nsec < 0) {
+		/* Perform borrow operation */
+		diff.tv_sec -= 1;
+		diff.tv_nsec += NSEC_PER_SEC;
+	}
+
+	/*
+	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
+	 * operation. diff.tv_sec is always positive.
+	 */
+	assert(diff.tv_sec >= 0 && diff.tv_nsec >= 0);
+	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
 }
 
 static
-- 
2.17.1

Re: [PATCH lttng-tools v2] Fix: timeout for connect is not respected

[Mathieu Desnoyers]

----- On Nov 9, 2018, at 1:52 PM, Jonathan Rajotte jonathan.rajotte-julien@efficios.com wrote:

>> > diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
>> > index e0b3e7a96..94a3c3a67 100644
>> > --- a/src/common/sessiond-comm/inet.c
>> > +++ b/src/common/sessiond-comm/inet.c
>> > @@ -119,16 +119,28 @@ static
>> > unsigned long time_diff_ms(struct timespec *time_a,
>> > 		struct timespec *time_b)
>> > {
>> > -	time_t sec_diff;
>> > -	long nsec_diff;
>> > -	unsigned long result_ms;
>> > +	struct timespec diff;
>> > 
>> > -	sec_diff = time_a->tv_sec - time_b->tv_sec;
>> > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
>> > +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
>> > +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
>> > 
>> > -	result_ms = sec_diff * MSEC_PER_SEC;
>> > -	result_ms += nsec_diff / NSEC_PER_MSEC;
>> > -	return result_ms;
>> > +	if (diff.tv_nsec < 0) {
>> > +		/* Perform borrow operation */
>> > +		diff.tv_sec -= 1;
>> > +		diff.tv_nsec += NSEC_PER_SEC;
>> > +	}
>> > +
>> > +	if (diff.tv_sec < 0) {
>> > +		/* We went back in time. Put all to zero */
>> > +		diff.tv_sec = 0;
>> > +		diff.tv_nsec = 0;
>> > +	}
>> 
>> Why would a diff operation implicitly floor everything to 0 when
>> time_a < time_b ?
> 
> Sure I can implement it as a real difference without the same "assumptions" of
> time_a > time_b since we are comparing passing time.
> 
> Make sure to tell this to past Mathieu ;).

I'll try, as soon as I find a time machine :)

> 
>> 
>> > +
>> > +	/*
>> > +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
>> > +	 * operation. diff.tb_sec is always positive.
>> 
>> tb_sec -> tv_nsec
> 
> Fixed.
> 
>> 
>> 
>> > +	 */
>> > +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
>> 
>> How do we handle time delta difference that overflows unsigned long ?
> 
> Let's do a quick calculation. Correct me if anything is wrong here.
> 
> How many msec does a unsigned long hold?
> A long is *minimum* 32 bits. Yielding a maximum value of 4294967295. [1]
> 
> Lets assume that the nanoseconds part of the diff struct is maxed out. The
> nanoseconds part yield 99 ms.
> 
> 4294967295 - 99 = 4294967196ms
> 
> How big would the second field of diff be a problem?
> 
> 4294967196/ MSEC_PER_SEC = 4294967s or 49 days.
> 
> I would err on the side of "that it's enough". We also have to take into
> account that for this to happen either something is going horribly wrong during
> getting the current time or the user purposely set the timeout to this value
> since we sample every 200ms.
> Which at this point goes into the PEBKAC category.
> 
> Do you agree?

It would be cleaner to return the result in an output parameter, return 0 on success,
negative error on error, and check for errors in the caller.

Thus, we can figure out if the result end up overflowing unsigned long, and return
an error accordingly.

Thanks,

Mathieu


> 
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf p.22 §5.2.4.2.1
> 
>> 
>> > }
>> > 
>> > static
>> > diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
>> > index dfb5fc5d1..07043e93f 100644
>> > --- a/src/common/sessiond-comm/inet6.c
>> > +++ b/src/common/sessiond-comm/inet6.c
>> > @@ -117,16 +117,28 @@ static
>> > unsigned long time_diff_ms(struct timespec *time_a,
>> > 		struct timespec *time_b)
>> 
>> This should be moved to a common implementation file. It's a cut and
>> paste from inet.c.
> 
> Past Mathieu did not seems to bother here.
> 
> I'm open into doing this in two patches since this should be backported. Do you
> have a suggestion regarding where to put it?
> 
>> 
>> Thanks,
>> 
>> Mathieu
>> 
>> > {
>> > -	time_t sec_diff;
>> > -	long nsec_diff;
>> > -	unsigned long result_ms;
>> > +	struct timespec diff;
>> > 
>> > -	sec_diff = time_a->tv_sec - time_b->tv_sec;
>> > -	nsec_diff = time_a->tv_nsec - time_b->tv_nsec;
>> > +	diff.tv_sec = time_a->tv_sec - time_b->tv_sec;
>> > +	diff.tv_nsec = time_a->tv_nsec - time_b->tv_nsec;
>> > 
>> > -	result_ms = sec_diff * MSEC_PER_SEC;
>> > -	result_ms += nsec_diff / NSEC_PER_MSEC;
>> > -	return result_ms;
>> > +	if (diff.tv_nsec < 0) {
>> > +		/* Perform borrow operation */
>> > +		diff.tv_sec -= 1;
>> > +		diff.tv_nsec += NSEC_PER_SEC;
>> > +	}
>> > +
>> > +	if (diff.tv_sec < 0) {
>> > +		/* We went back in time. Put all to zero */
>> > +		diff.tv_sec = 0;
>> > +		diff.tv_nsec = 0;
>> > +	}
>> > +
>> > +	/*
>> > +	 * Transform into MSEC, diff.tv_nsec is always positive due to borrow
>> > +	 * operation. diff.tb_sec is always positive.
>> > +	 */
>> > +	return (diff.tv_sec * MSEC_PER_SEC) + (diff.tv_nsec / NSEC_PER_MSEC);
>> > }
>> > 
>> > static
>> > --
>> > 2.17.1
>> > 
>> > _______________________________________________
>> > lttng-dev mailing list
>> > lttng-dev@lists.lttng.org
>> > https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com
> 
> --
> Jonathan Rajotte-Julien
> EfficiOS

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev