Discussion:
[lttng-dev] [PATCH lttng-tools 2/2] Fix: max_t/min_t macros are missing cast on input
Mathieu Desnoyers
2018-11-09 22:27:49 UTC
Permalink
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 <***@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
Jonathan Rajotte-Julien
2018-11-09 22:34:07 UTC
Permalink
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().
---
src/common/common.h | 5 +++++
src/common/sessiond-comm/inet.c | 28 ++++++++--------------------
src/common/sessiond-comm/inet6.c | 28 ++++++++--------------------
src/common/utils.c | 29 +++++++++++++++++++++++++++++
4 files changed, 50 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"
+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..c4759f68 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,30 @@ int utils_show_help(int section, const char *page_name,
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 * INT64_C(1000000000) + (uint64_t) t1.tv_nsec;
+ uint64_t ts2 = (uint64_t) t2.tv_sec * INT64_C(1000000000) + (uint64_t) t2.tv_nsec;
Use NSEC_PER_SEC here.
+ uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
+ struct timespec res;
+
+ res.tv_sec = diff / INT64_C(1000000000);
Use NSEC_PER_SEC here.
+ res.tv_nsec = diff % INT64_C(1000000000);
Use NSEC_PER_SEC here.
+ return res;
+}
--
2.11.0
--
Jonathan Rajotte-Julien
EfficiOS
Mathieu Desnoyers
2018-11-09 22:41:17 UTC
Permalink
----- On Nov 9, 2018, at 5:34 PM, Jonathan Rajotte jonathan.rajotte-***@efficios.com wrote:
[...]
Post by Jonathan Rajotte-Julien
+
+LTTNG_HIDDEN
+struct timespec timespec_abs_diff(struct timespec t1, struct timespec t2)
+{
+ uint64_t ts1 = (uint64_t) t1.tv_sec * INT64_C(1000000000) + (uint64_t) t1.tv_nsec;
+ uint64_t ts2 = (uint64_t) t2.tv_sec * INT64_C(1000000000) + (uint64_t) t2.tv_nsec;
Use NSEC_PER_SEC here.
+ uint64_t diff = max(ts1, ts2) - min(ts1, ts2);
+ struct timespec res;
+
+ res.tv_sec = diff / INT64_C(1000000000);
Use NSEC_PER_SEC here.
+ res.tv_nsec = diff % INT64_C(1000000000);
Use NSEC_PER_SEC here.
Fixing in v2, thanks,

Mathieu
Post by Jonathan Rajotte-Julien
+ return res;
+}
--
2.11.0
--
Jonathan Rajotte-Julien
EfficiOS
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Loading...