[PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[Mathieu Desnoyers]

The semantic expected from max_t and min_t is to perform the max/min
comparison in the type provided as first parameter.

Cast the input parameters to the proper type before comparing them,
rather than after. There is no more need to cast the result of the
expression now that both inputs are cast to the right type.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 src/common/macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/macros.h b/src/common/macros.h
index c521aacd..461202ff 100644
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -72,7 +72,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef max_t
-#define max_t(type, a, b)	((type) max(a, b))
+#define max_t(type, a, b)	max((type) a, (type) b)
 #endif
 
 #ifndef min
@@ -80,7 +80,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef min_t
-#define min_t(type, a, b)	((type) min(a, b))
+#define min_t(type, a, b)	min((type) a, (type) b)
 #endif
 
 #ifndef LTTNG_PACKED
-- 
2.11.0

Re: [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)

[Jonathan Rajotte-Julien]

As discussed on IRC, could you make sure to test it manually at least once since
the problematic scenario is never tested by regression test.
Looks much better than my first attempt but we never know.

You can force delay locally using tc.

Something like:

    LTTNG_NETWORK_SOCKET_TIMEOUT=4000 lttng-sessiond -b -vvv
    lttng-relayd -b
    lttng create --live
    sudo tc qdisc add dev lo root netem delay 10000ms
    lttng enable-event -u -a
    #Should see errors of connection timeout taking around 4 secs, validate using
    #the logs.

    sudo tc qdisc del dev lo root netem delay 10000ms

You can also do the opposite for delays to validate working connect that take a
long time.

As Simon pointed out, unit tests would be even better for these new functions.
Something like test_utils_expand_path.c perhaps? A regression test would be nice
but I'm not sure how we would do it based on the current test framework.

Other comments inline for stuff I did not see the first time.

On Fri, Nov 09, 2018 at 05:40:46PM -0500, Mathieu Desnoyers wrote:
> The nanoseconds part of the timespec struct time_a is not always
> bigger than time_b since it wrap around each seconds.
> 
> Use 64-bit arithmetic to perform the difference.
> 
> Merge/move duplicated code into utils.c.
> 
> This function is really doing two things. Split it into timespec_to_ms()
> and timespec_abs_diff().
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> Changes since v1:
> - Use NSEC_PER_SEC constant.
> ---
>  src/common/common.h              |  5 +++++
>  src/common/sessiond-comm/inet.c  | 28 ++++++++--------------------
>  src/common/sessiond-comm/inet6.c | 28 ++++++++--------------------
>  src/common/utils.c               | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/src/common/common.h b/src/common/common.h
> index 41eb0361..48f39e0e 100644
> --- a/src/common/common.h
> +++ b/src/common/common.h
> @@ -19,9 +19,14 @@
>  #ifndef _COMMON_H
>  #define _COMMON_H
>  
> +#include <time.h>
> +
>  #include "error.h"
>  #include "macros.h"
>  #include "runas.h"
>  #include "readwrite.h"
>  

Given the current format of common.h, wouldn't it be better to have a
timespec_ops.h file with these declaration in it? I guess it can be done in a
later commit to minimize backporting since it is not necessary for the fix.

Please add a comment explaining behaviour on error and what it represent.

> +int timespec_to_ms(struct timespec ts, unsigned long *ms);
> +struct timespec timespec_abs_diff(struct timespec ts_a, struct timespec ts_b);
> +
>  #endif /* _COMMON_H */
> diff --git a/src/common/sessiond-comm/inet.c b/src/common/sessiond-comm/inet.c
> index e0b3e7a9..76a3b7ff 100644
> --- a/src/common/sessiond-comm/inet.c
> +++ b/src/common/sessiond-comm/inet.c
> @@ -112,31 +112,13 @@ int connect_no_timeout(struct lttcomm_sock *sock)
>  			sizeof(sock->sockaddr.addr.sin));
>  }
>  
> -/*
> - * Return time_a - time_b  in milliseconds.
> - */
> -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;
> -
> -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = 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;
> -}
> -
>  static
>  int connect_with_timeout(struct lttcomm_sock *sock)
>  {
>  	unsigned long timeout = lttcomm_get_network_timeout();
>  	int ret, flags, connect_ret;
>  	struct timespec orig_time, cur_time;
> +	unsigned long diff_ms;
>  
>  	ret = fcntl(sock->fd, F_GETFL, 0);
>  	if (ret == -1) {
> @@ -217,7 +199,13 @@ int connect_with_timeout(struct lttcomm_sock *sock)
>  			connect_ret = ret;
>  			goto error;
>  		}
> -	} while (time_diff_ms(&cur_time, &orig_time) < timeout);
> +		if (timespec_to_ms(timespec_abs_diff(cur_time, orig_time), &diff_ms) < 0) {
> +			ERR("timespec_to_ms input overflows milliseconds output");
> +			errno = EOVERFLOW;
> +			connect_ret = -1;
> +			goto error;
> +		}
> +	} while (diff_ms < timeout);
>  
>  	/* Timeout */
>  	errno = ETIMEDOUT;
> diff --git a/src/common/sessiond-comm/inet6.c b/src/common/sessiond-comm/inet6.c
> index dfb5fc5d..5c1ac2e0 100644
> --- a/src/common/sessiond-comm/inet6.c
> +++ b/src/common/sessiond-comm/inet6.c
> @@ -110,31 +110,13 @@ int connect_no_timeout(struct lttcomm_sock *sock)
>  			sizeof(sock->sockaddr.addr.sin6));
>  }
>  
> -/*
> - * Return time_a - time_b  in milliseconds.
> - */
> -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;
> -
> -	sec_diff = time_a->tv_sec - time_b->tv_sec;
> -	nsec_diff = 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;
> -}
> -
>  static
>  int connect_with_timeout(struct lttcomm_sock *sock)
>  {
>  	unsigned long timeout = lttcomm_get_network_timeout();
>  	int ret, flags, connect_ret;
>  	struct timespec orig_time, cur_time;
> +	unsigned long diff_ms;
>  
>  	ret = fcntl(sock->fd, F_GETFL, 0);
>  	if (ret == -1) {
> @@ -216,7 +198,13 @@ int connect_with_timeout(struct lttcomm_sock *sock)
>  			connect_ret = ret;
>  			goto error;
>  		}
> -	} while (time_diff_ms(&cur_time, &orig_time) < timeout);
> +		if (timespec_to_ms(timespec_abs_diff(cur_time, orig_time), &diff_ms) < 0) {
> +			ERR("timespec_to_ms input overflows milliseconds output");
> +			errno = EOVERFLOW;
> +			connect_ret = -1;
> +			goto error;
> +		}
> +	} while (diff_ms < timeout);
>  
>  	/* Timeout */
>  	errno = ETIMEDOUT;
> diff --git a/src/common/utils.c b/src/common/utils.c
> index 3442bef8..5b4e1d3c 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -31,6 +31,7 @@
>  #include <pwd.h>
>  #include <sys/file.h>
>  #include <unistd.h>
> +#include <stdint.h>
>  
>  #include <common/common.h>
>  #include <common/runas.h>
> @@ -41,6 +42,7 @@
>  
>  #include "utils.h"
>  #include "defaults.h"
> +#include "time.h"
>  
>  /*
>   * Return a partial realpath(3) of the path even if the full path does not
> @@ -1645,3 +1647,32 @@ int utils_show_help(int section, const char *page_name,
>  end:
>  	return ret;
>  }
> +
> +LTTNG_HIDDEN
> +int timespec_to_ms(struct timespec ts, unsigned long *ms)
> +{
> +	unsigned long res;
> +
> +	if (ts.tv_sec + 1 > ULONG_MAX / MSEC_PER_SEC) {
> +		return -1;
> +	}
> +	res = ts.tv_sec * MSEC_PER_SEC;
> +	res += ts.tv_nsec / NSEC_PER_MSEC;
> +	*ms = res;
> +	return 0;
> +}
> +
> +LTTNG_HIDDEN
> +struct timespec timespec_abs_diff(struct timespec t1, struct timespec t2)
> +{
> +	uint64_t ts1 = (uint64_t) t1.tv_sec * (uint64_t) NSEC_PER_SEC +
> +			(uint64_t) t1.tv_nsec;
> +	uint64_t ts2 = (uint64_t) t2.tv_sec * (uint64_t) NSEC_PER_SEC +
> +			(uint64_t) t2.tv_nsec;
> +	uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
> +	struct timespec res;
> +
> +	res.tv_sec = diff / (uint64_t) NSEC_PER_SEC;
> +	res.tv_nsec = diff % (uint64_t) NSEC_PER_SEC;
> +	return res;
> +}
> -- 
> 2.11.0
> 

-- 
Jonathan Rajotte-Julien
EfficiOS

Re: [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)

[Jonathan Rajotte-Julien]

> diff --git a/src/common/utils.c b/src/common/utils.c
> index 3442bef8..5b4e1d3c 100644
> --- a/src/common/utils.c
> +++ b/src/common/utils.c
> @@ -31,6 +31,7 @@
>  #include <pwd.h>
>  #include <sys/file.h>
>  #include <unistd.h>
> +#include <stdint.h>
>  
>  #include <common/common.h>
>  #include <common/runas.h>
> @@ -41,6 +42,7 @@
>  
>  #include "utils.h"
>  #include "defaults.h"
> +#include "time.h"
>  
>  /*
>   * Return a partial realpath(3) of the path even if the full path does not
> @@ -1645,3 +1647,32 @@ int utils_show_help(int section, const char *page_name,
>  end:
>  	return ret;
>  }
> +
> +LTTNG_HIDDEN
> +int timespec_to_ms(struct timespec ts, unsigned long *ms)
> +{
> +	unsigned long res;
> +
> +	if (ts.tv_sec + 1 > ULONG_MAX / MSEC_PER_SEC) {
> +		return -1;
> +	}

This is not accurate. It return -1 for the all the following valid timespec:

   tv_sec = 4294967
   0 < tv_nsec < 295000000

You will need to check multiplication and addition separately.

> +	res = ts.tv_sec * MSEC_PER_SEC;
> +	res += ts.tv_nsec / NSEC_PER_MSEC;
> +	*ms = res;
> +	return 0;
> +}
> +
> +LTTNG_HIDDEN
> +struct timespec timespec_abs_diff(struct timespec t1, struct timespec t2)
> +{
> +	uint64_t ts1 = (uint64_t) t1.tv_sec * (uint64_t) NSEC_PER_SEC +
> +			(uint64_t) t1.tv_nsec;
> +	uint64_t ts2 = (uint64_t) t2.tv_sec * (uint64_t) NSEC_PER_SEC +
> +			(uint64_t) t2.tv_nsec;
> +	uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
> +	struct timespec res;
> +
> +	res.tv_sec = diff / (uint64_t) NSEC_PER_SEC;
> +	res.tv_nsec = diff % (uint64_t) NSEC_PER_SEC;
> +	return res;
> +}
> -- 
> 2.11.0
> 

-- 
Jonathan Rajotte-Julien
EfficiOS

Re: [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)

[Mathieu Desnoyers]

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

>> diff --git a/src/common/utils.c b/src/common/utils.c
>> index 3442bef8..5b4e1d3c 100644
>> --- a/src/common/utils.c
>> +++ b/src/common/utils.c

[...]

>> +LTTNG_HIDDEN
>> +int timespec_to_ms(struct timespec ts, unsigned long *ms)
>> +{
>> +	unsigned long res;
>> +
>> +	if (ts.tv_sec + 1 > ULONG_MAX / MSEC_PER_SEC) {
>> +		return -1;
>> +	}
> 
> This is not accurate. It return -1 for the all the following valid timespec:
> 
>   tv_sec = 4294967
>   0 < tv_nsec < 295000000
> 
> You will need to check multiplication and addition separately.

Good point! Changing the check for:

        if (ts.tv_sec + ts.tv_nsec / NSEC_PER_SEC > ULONG_MAX / MSEC_PER_SEC) {
                return -1;
        }

Indeed it appears to be valid to have tv_nsec greater than (1000000000-1)

Thanks,

Mathieu

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

Re: [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)

[Mathieu Desnoyers]

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

> As discussed on IRC, could you make sure to test it manually at least once since
> the problematic scenario is never tested by regression test.
> Looks much better than my first attempt but we never know.
> 
> You can force delay locally using tc.
> 
> Something like:
> 
>    LTTNG_NETWORK_SOCKET_TIMEOUT=4000 lttng-sessiond -b -vvv
>    lttng-relayd -b
>    lttng create --live
>    sudo tc qdisc add dev lo root netem delay 10000ms
>    lttng enable-event -u -a
>    #Should see errors of connection timeout taking around 4 secs, validate using
>    #the logs.
> 
>    sudo tc qdisc del dev lo root netem delay 10000ms
> 
> You can also do the opposite for delays to validate working connect that take a
> long time.

Let's test it on Monday. Meanwhile I'll send an updated version taking your
comments into account.

> 
> As Simon pointed out, unit tests would be even better for these new functions.
> Something like test_utils_expand_path.c perhaps? A regression test would be nice
> but I'm not sure how we would do it based on the current test framework.
> 
> Other comments inline for stuff I did not see the first time.
> 
> On Fri, Nov 09, 2018 at 05:40:46PM -0500, Mathieu Desnoyers wrote:
>> The nanoseconds part of the timespec struct time_a is not always
>> bigger than time_b since it wrap around each seconds.
>> 
>> Use 64-bit arithmetic to perform the difference.
>> 
>> Merge/move duplicated code into utils.c.
>> 
>> This function is really doing two things. Split it into timespec_to_ms()
>> and timespec_abs_diff().
>> 
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> ---
>> Changes since v1:
>> - Use NSEC_PER_SEC constant.
>> ---
>>  src/common/common.h              |  5 +++++
>>  src/common/sessiond-comm/inet.c  | 28 ++++++++--------------------
>>  src/common/sessiond-comm/inet6.c | 28 ++++++++--------------------
>>  src/common/utils.c               | 31 +++++++++++++++++++++++++++++++
>>  4 files changed, 52 insertions(+), 40 deletions(-)
>> 
>> diff --git a/src/common/common.h b/src/common/common.h
>> index 41eb0361..48f39e0e 100644
>> --- a/src/common/common.h
>> +++ b/src/common/common.h
>> @@ -19,9 +19,14 @@
>>  #ifndef _COMMON_H
>>  #define _COMMON_H
>>  
>> +#include <time.h>
>> +
>>  #include "error.h"
>>  #include "macros.h"
>>  #include "runas.h"
>>  #include "readwrite.h"
>>  
> 
> Given the current format of common.h, wouldn't it be better to have a
> timespec_ops.h file with these declaration in it? I guess it can be done in a
> later commit to minimize backporting since it is not necessary for the fix.

We already have a src/common/time.h. I'll move it there. Good point!

> 
> Please add a comment explaining behaviour on error and what it represent.

Done.

Next time, to facilitate reading through your comments, please delete diff hunks
that are not relevant to the comments when you reply. This helps everyone reading
through the comments without having to scroll down many pages of code. Otherwise
it's just too easy to miss a small one-liner comment in a forest of diff.

Thanks,

Mathieu


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

Re: [PATCH lttng-tools 1/2] Fix: Connect timeout arithmetic in inet/inet6 (v2)

[Mathieu Desnoyers]

----- On Nov 10, 2018, at 9:20 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Nov 9, 2018, at 6:13 PM, Jonathan Rajotte
> jonathan.rajotte-julien@efficios.com wrote:
> 
>> As discussed on IRC, could you make sure to test it manually at least once since
>> the problematic scenario is never tested by regression test.
>> Looks much better than my first attempt but we never know.
>> 
>> You can force delay locally using tc.
>> 
>> Something like:
>> 
>>    LTTNG_NETWORK_SOCKET_TIMEOUT=4000 lttng-sessiond -b -vvv
>>    lttng-relayd -b
>>    lttng create --live
>>    sudo tc qdisc add dev lo root netem delay 10000ms
>>    lttng enable-event -u -a
>>    #Should see errors of connection timeout taking around 4 secs, validate using
>>    #the logs.
>> 
>>    sudo tc qdisc del dev lo root netem delay 10000ms
>> 
>> You can also do the opposite for delays to validate working connect that take a
>> long time.
> 
> Let's test it on Monday. Meanwhile I'll send an updated version taking your
> comments into account.
> 

It seems to work fine with my patch applied.

Thanks,

Mathieu

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

Re: [PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[Jérémie Galarneau]

Merged all the way back to 2.9.

Thanks!
Jérémie

On Tue, Nov 13, 2018 at 12:12:21PM -0500, Mathieu Desnoyers wrote:
> The semantic expected from max_t and min_t is to perform the max/min
> comparison in the type provided as first parameter.
> 
> Cast the input parameters to the proper type before comparing them,
> rather than after. There is no more need to cast the result of the
> expression now that both inputs are cast to the right type.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  src/common/macros.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/common/macros.h b/src/common/macros.h
> index c521aacd..461202ff 100644
> --- a/src/common/macros.h
> +++ b/src/common/macros.h
> @@ -72,7 +72,7 @@ void *zmalloc(size_t len)
>  #endif
>  
>  #ifndef max_t
> -#define max_t(type, a, b)	((type) max(a, b))
> +#define max_t(type, a, b)	max((type) a, (type) b)
>  #endif
>  
>  #ifndef min
> @@ -80,7 +80,7 @@ void *zmalloc(size_t len)
>  #endif
>  
>  #ifndef min_t
> -#define min_t(type, a, b)	((type) min(a, b))
> +#define min_t(type, a, b)	min((type) a, (type) b)
>  #endif
>  
>  #ifndef LTTNG_PACKED
> -- 
> 2.11.0
> 

[PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[Mathieu Desnoyers]

The semantic expected from max_t and min_t is to perform the max/min
comparison in the type provided as first parameter.

Cast the input parameters to the proper type before comparing them,
rather than after. There is no more need to cast the result of the
expression now that both inputs are cast to the right type.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 src/common/macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/macros.h b/src/common/macros.h
index c521aacd..461202ff 100644
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -72,7 +72,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef max_t
-#define max_t(type, a, b)	((type) max(a, b))
+#define max_t(type, a, b)	max((type) a, (type) b)
 #endif
 
 #ifndef min
@@ -80,7 +80,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef min_t
-#define min_t(type, a, b)	((type) min(a, b))
+#define min_t(type, a, b)	min((type) a, (type) b)
 #endif
 
 #ifndef LTTNG_PACKED
-- 
2.11.0

[PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[Mathieu Desnoyers]

The semantic expected from max_t and min_t is to perform the max/min
comparison in the type provided as first parameter.

Cast the input parameters to the proper type before comparing them,
rather than after. There is no more need to cast the result of the
expression now that both inputs are cast to the right type.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 src/common/macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/macros.h b/src/common/macros.h
index c521aacd..461202ff 100644
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -72,7 +72,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef max_t
-#define max_t(type, a, b)	((type) max(a, b))
+#define max_t(type, a, b)	max((type) a, (type) b)
 #endif
 
 #ifndef min
@@ -80,7 +80,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef min_t
-#define min_t(type, a, b)	((type) min(a, b))
+#define min_t(type, a, b)	min((type) a, (type) b)
 #endif
 
 #ifndef LTTNG_PACKED
-- 
2.11.0

[PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input

[Mathieu Desnoyers]

The semantic expected from max_t and min_t is to perform the max/min
comparison in the type provided as first parameter.

Cast the input parameters to the proper type before comparing them,
rather than after. There is no more need to cast the result of the
expression now that both inputs are cast to the right type.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 src/common/macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/common/macros.h b/src/common/macros.h
index c521aacd..461202ff 100644
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -72,7 +72,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef max_t
-#define max_t(type, a, b)	((type) max(a, b))
+#define max_t(type, a, b)	max((type) a, (type) b)
 #endif
 
 #ifndef min
@@ -80,7 +80,7 @@ void *zmalloc(size_t len)
 #endif
 
 #ifndef min_t
-#define min_t(type, a, b)	((type) min(a, b))
+#define min_t(type, a, b)	min((type) a, (type) b)
 #endif
 
 #ifndef LTTNG_PACKED
-- 
2.11.0