subject:"\[PATCH\] packages\: pagekitec\: fix crashing due to musl time64"

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

2022-06-04 Thread Lukas Zeller

> On 3 Jun 2022, at 23:10, Karl Palsson  wrote:
> 
> [...] Ideally, even make the patch against upstream:
> https://github.com/pagekite/libpagekite so they are involved and
> can help with it going forward, especially for cases like this,
> where it's not anything that's OpenWrt related.

Thanks for the hint. Yes, this makes more sense, so I just posted a PR
there.

I modified the fix a bit as Rosen Penev is correct to criticise the
use of %lld together with an uint64_t cast. It would replace one mismatch
with another, just less critical one...

However, I chose to keep %lld and cast to long long instead, as we
generally don't know what time_t really is. As long
as there is no PRItime_t, this seems more generic to me than uint64_t.

> I'll try and poke upstream about this personally too.

Thanks!

signature.asc
Description: Message signed with OpenPGP
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

2022-06-03 Thread Rosen Penev

On Fri, Jun 3, 2022 at 10:24 AM  wrote:
>
> From: Lukas Zeller 
>
> [Please note: this is my first attempt at submitting a patch,
> please apologize/advise if something is not as it should be. I tried to
> follow the guideline in the wiki]
https://github.com/openwrt/packages <-- PR there
>
> upstream pagekitec/libpagekite is not compatible with musl>=1.2.0 time64
> (time_t being 64 bit rather than 32 bit) due to several format strings using 
> "%d"
> for a time_t parameter.
>
> With time_t becoming 64bit, these (mostly log output) printfs display invalid
> numbers, or even crash when mixed with "%s" format specifiers.
>
> Most prominently visible case is the "Tick!" message which causes any
> real world use of pagekitec/libpagekite crash immediately.
>
> This patch now uses "%lld" with an explicit cast of the arguments to uint64_t.
> The cast ensures that the code is still safe to compile with 32 bit time_t
> (but introduces a slight performance penalty in that case).
>
> Signed-off-by: Lukas Zeller 
> ---
>  net/pagekitec/patches/500-time64-fixes.patch | 70 
>  1 file changed, 70 insertions(+)
>  create mode 100644 net/pagekitec/patches/500-time64-fixes.patch
>
> diff --git a/net/pagekitec/patches/500-time64-fixes.patch 
> b/net/pagekitec/patches/500-time64-fixes.patch
> new file mode 100644
> index 0..a052a16c4
> --- /dev/null
> +++ b/net/pagekitec/patches/500-time64-fixes.patch
> @@ -0,0 +1,70 @@
> +--- a/libpagekite/pklogging.c
>  b/libpagekite/pklogging.c
> +@@ -208,9 +208,9 @@ void pk_dump_conn(char* prefix, struct p
> +   if (conn->sockfd < 0) return;
> +
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sockfd: %d", prefix, conn->sockfd);
> +-  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %x (%ds ago)", prefix,
> +-   conn->activity,
> +-   pk_time(0) - conn->activity);
> ++  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %llx (%llds ago)", prefix,
ll == long long. PRIu64 == uint64_t.

Same issue throughout this patch.
> ++   (uint64_t)conn->activity,
> ++   (uint64_t)(pk_time(0) - conn->activity));
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_bytes: %d", prefix, 
> conn->read_bytes);
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_kb: %d", prefix, conn->read_kb);
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sent_kb: %d", prefix, conn->sent_kb);
> +@@ -281,8 +281,8 @@ void pk_dump_state(struct pk_manager* pk
> +   pk_log(LL, "pk_manager/kite_max: %d", pkm->kite_max);
> +   pk_log(LL, "pk_manager/tunnel_max: %d", pkm->tunnel_max);
> +   pk_log(LL, "pk_manager/be_conn_max: %d", pkm->be_conn_max);
> +-  pk_log(LL, "pk_manager/last_world_update: %x", pkm->last_world_update);
> +-  pk_log(LL, "pk_manager/next_tick: %d", pkm->next_tick);
> ++  pk_log(LL, "pk_manager/last_world_update: %llx", 
> (uint64_t)pkm->last_world_update);
> ++  pk_log(LL, "pk_manager/next_tick: %lld", (uint64_t)pkm->next_tick);
> +   pk_log(LL, "pk_manager/enable_timer: %d", 0 < pkm->enable_timer);
> +   pk_log(LL, "pk_manager/fancy_pagekite_net_rejection_url: %s", 
> pkm->fancy_pagekite_net_rejection_url);
> +   pk_log(LL, "pk_manager/want_spare_frontends: %d", 
> pkm->want_spare_frontends);
> +--- a/libpagekite/pkmanager.c
>  b/libpagekite/pkmanager.c
> +@@ -1070,8 +1070,8 @@ static void pkm_tick_cb(EV_P_ ev_async*
> + pkm->timer.repeat = pkm->next_tick;
> + ev_timer_again(pkm->loop, &(pkm->timer));
> + pk_log(PK_LOG_MANAGER_INFO,
> +-   "Tick!  [repeating=%s, next=%d, status=%d, tunnels=%d, v=%s]",
> +-   pkm->enable_timer ? "yes" : "no", pkm->next_tick,
> ++   "Tick!  [repeating=%s, next=%lld, status=%d, tunnels=%d, v=%s]",
> ++   pkm->enable_timer ? "yes" : "no", (uint64_t)pkm->next_tick,
> +pkm->status, pk_state.live_tunnels, PK_VERSION);
> +
> + /* We slow down exponentially by default... */
> +@@ -1122,8 +1122,8 @@ static void pkm_tick_cb(EV_P_ ev_async*
> +   fe->last_ping = now;
> +   pkc_write(&(fe->conn), ping, pingsize);
> +   pk_log(PK_LOG_TUNNEL_DATA,
> +-  "%d: Sent PING (idle=%ds>%ds)",
> +-  fe->conn.sockfd, now - fe->conn.activity, now - inactive);
> ++  "%d: Sent PING (idle=%llds>%llds)",
> ++  fe->conn.sockfd, (uint64_t)(now - fe->conn.activity), 
> (uint64_t)(now - inactive));
> +   next_tick = 1 + pkm->housekeeping_interval_min;
> + }
> +   }
> +--- a/libpagekite/pkproto.c
>  b/libpagekite/pkproto.c
> +@@ -577,7 +577,7 @@ int pk_make_salt(char* salt) {
> + char* pk_sign(const char* token, const char* secret, time_t ts,
> +   const char* payload, int length, char *buffer)
> + {
> +-  char tbuffer[128], tsbuf[16], scratch[10240];
> ++  char tbuffer[128], tsbuf[32], scratch[10240];
> +
> +   PK_TRACE_FUNCTION;
> +
> +@@ -606,7 +606,7 @@ char* pk_sign(const char* token, const c
> +
> +   /* Optionally embed a timestamp to the

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

2022-06-03 Thread Karl Palsson


Classically, as a patch for a package, this should be filed here:
https://github.com/openwrt/packages/pulls

Ideally, even make the patch against upstream:
https://github.com/pagekite/libpagekite so they are involved and
can help with it going forward, especially for cases like this,
where it's not anything that's OpenWrt related.

I'll try and poke upstream about this personally too.

Sincerely,
Karl Palsson

l...@plan44.ch wrote:
> From: Lukas Zeller 
> 
> [Please note: this is my first attempt at submitting a patch,
> please apologize/advise if something is not as it should be. I
> tried to follow the guideline in the wiki]
> 
> upstream pagekitec/libpagekite is not compatible with
> musl>=1.2.0 time64 (time_t being 64 bit rather than 32 bit) due
> to several format strings using "%d" for a time_t parameter.
> 
> With time_t becoming 64bit, these (mostly log output) printfs
> display invalid numbers, or even crash when mixed with "%s"
> format specifiers.
> 
> Most prominently visible case is the "Tick!" message which
> causes any real world use of pagekitec/libpagekite crash
> immediately.
> 
> This patch now uses "%lld" with an explicit cast of the
> arguments to uint64_t. The cast ensures that the code is still
> safe to compile with 32 bit time_t (but introduces a slight
> performance penalty in that case).
> 
> Signed-off-by: Lukas Zeller 
> ---
>  net/pagekitec/patches/500-time64-fixes.patch | 70 
>  1 file changed, 70 insertions(+)
>  create mode 100644 net/pagekitec/patches/500-time64-fixes.patch
> 
> diff --git a/net/pagekitec/patches/500-time64-fixes.patch
> b/net/pagekitec/patches/500-time64-fixes.patch new file mode
> 100644 index 0..a052a16c4
> --- /dev/null
> +++ b/net/pagekitec/patches/500-time64-fixes.patch
> @@ -0,0 +1,70 @@
> +--- a/libpagekite/pklogging.c
>  b/libpagekite/pklogging.c
> +@@ -208,9 +208,9 @@ void pk_dump_conn(char* prefix, struct p
> +   if (conn->sockfd < 0) return;
> + 
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sockfd: %d", prefix, conn->sockfd);
> +-  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %x (%ds ago)", prefix,
> +-   conn->activity,
> +-   pk_time(0) - conn->activity);
> ++  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %llx (%llds ago)", prefix,
> ++   (uint64_t)conn->activity,
> ++   (uint64_t)(pk_time(0) - conn->activity));
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_bytes: %d", prefix, 
> conn->read_bytes);
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_kb: %d", prefix, conn->read_kb);
> +   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sent_kb: %d", prefix, conn->sent_kb);
> +@@ -281,8 +281,8 @@ void pk_dump_state(struct pk_manager* pk
> +   pk_log(LL, "pk_manager/kite_max: %d", pkm->kite_max);
> +   pk_log(LL, "pk_manager/tunnel_max: %d", pkm->tunnel_max);
> +   pk_log(LL, "pk_manager/be_conn_max: %d", pkm->be_conn_max);
> +-  pk_log(LL, "pk_manager/last_world_update: %x", pkm->last_world_update);
> +-  pk_log(LL, "pk_manager/next_tick: %d", pkm->next_tick);
> ++  pk_log(LL, "pk_manager/last_world_update: %llx", 
> (uint64_t)pkm->last_world_update);
> ++  pk_log(LL, "pk_manager/next_tick: %lld", (uint64_t)pkm->next_tick);
> +   pk_log(LL, "pk_manager/enable_timer: %d", 0 < pkm->enable_timer);
> +   pk_log(LL, "pk_manager/fancy_pagekite_net_rejection_url: %s", 
> pkm->fancy_pagekite_net_rejection_url);
> +   pk_log(LL, "pk_manager/want_spare_frontends: %d", 
> pkm->want_spare_frontends);
> +--- a/libpagekite/pkmanager.c
>  b/libpagekite/pkmanager.c
> +@@ -1070,8 +1070,8 @@ static void pkm_tick_cb(EV_P_ ev_async* 
> + pkm->timer.repeat = pkm->next_tick;
> + ev_timer_again(pkm->loop, &(pkm->timer));
> + pk_log(PK_LOG_MANAGER_INFO,
> +-   "Tick!  [repeating=%s, next=%d, status=%d, tunnels=%d, v=%s]",
> +-   pkm->enable_timer ? "yes" : "no", pkm->next_tick,
> ++   "Tick!  [repeating=%s, next=%lld, status=%d, tunnels=%d, v=%s]",
> ++   pkm->enable_timer ? "yes" : "no", (uint64_t)pkm->next_tick,
> +pkm->status, pk_state.live_tunnels, PK_VERSION);
> + 
> + /* We slow down exponentially by default... */
> +@@ -1122,8 +1122,8 @@ static void pkm_tick_cb(EV_P_ ev_async* 
> +   fe->last_ping = now;
> +   pkc_write(&(fe->conn), ping, pingsize);
> +   pk_log(PK_LOG_TUNNEL_DATA,
> +-  "%d: Sent PING (idle=%ds>%ds)",
> +-  fe->conn.sockfd, now - fe->conn.activity, now - inactive);
> ++  "%d: Sent PING (idle=%llds>%llds)",
> ++  fe->conn.sockfd, (uint64_t)(now - fe->conn.activity), 
> (uint64_t)(now - inactive));
> +   next_tick = 1 + pkm->housekeeping_interval_min;
> + }
> +   }
> +--- a/libpagekite/pkproto.c
>  b/libpagekite/pkproto.c
> +@@ -577,7 +577,7 @@ int pk_make_salt(char* salt) {
> + char* pk_sign(const char* token, const char* secret, time_t ts,
> +   const char* payload,

[PATCH] packages: pagekitec: fix crashing due to musl time64

2022-06-03 Thread luz

From: Lukas Zeller 

[Please note: this is my first attempt at submitting a patch,
please apologize/advise if something is not as it should be. I tried to
follow the guideline in the wiki]

upstream pagekitec/libpagekite is not compatible with musl>=1.2.0 time64
(time_t being 64 bit rather than 32 bit) due to several format strings using 
"%d"
for a time_t parameter.

With time_t becoming 64bit, these (mostly log output) printfs display invalid
numbers, or even crash when mixed with "%s" format specifiers.

Most prominently visible case is the "Tick!" message which causes any
real world use of pagekitec/libpagekite crash immediately.

This patch now uses "%lld" with an explicit cast of the arguments to uint64_t.
The cast ensures that the code is still safe to compile with 32 bit time_t
(but introduces a slight performance penalty in that case).

Signed-off-by: Lukas Zeller 
---
 net/pagekitec/patches/500-time64-fixes.patch | 70 
 1 file changed, 70 insertions(+)
 create mode 100644 net/pagekitec/patches/500-time64-fixes.patch

diff --git a/net/pagekitec/patches/500-time64-fixes.patch 
b/net/pagekitec/patches/500-time64-fixes.patch
new file mode 100644
index 0..a052a16c4
--- /dev/null
+++ b/net/pagekitec/patches/500-time64-fixes.patch
@@ -0,0 +1,70 @@
+--- a/libpagekite/pklogging.c
 b/libpagekite/pklogging.c
+@@ -208,9 +208,9 @@ void pk_dump_conn(char* prefix, struct p
+   if (conn->sockfd < 0) return;
+ 
+   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sockfd: %d", prefix, conn->sockfd);
+-  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %x (%ds ago)", prefix,
+-   conn->activity,
+-   pk_time(0) - conn->activity);
++  pk_log(PK_LOG_MANAGER_DEBUG, "%s/activity: %llx (%llds ago)", prefix,
++   (uint64_t)conn->activity,
++   (uint64_t)(pk_time(0) - conn->activity));
+   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_bytes: %d", prefix, conn->read_bytes);
+   pk_log(PK_LOG_MANAGER_DEBUG, "%s/read_kb: %d", prefix, conn->read_kb);
+   pk_log(PK_LOG_MANAGER_DEBUG, "%s/sent_kb: %d", prefix, conn->sent_kb);
+@@ -281,8 +281,8 @@ void pk_dump_state(struct pk_manager* pk
+   pk_log(LL, "pk_manager/kite_max: %d", pkm->kite_max);
+   pk_log(LL, "pk_manager/tunnel_max: %d", pkm->tunnel_max);
+   pk_log(LL, "pk_manager/be_conn_max: %d", pkm->be_conn_max);
+-  pk_log(LL, "pk_manager/last_world_update: %x", pkm->last_world_update);
+-  pk_log(LL, "pk_manager/next_tick: %d", pkm->next_tick);
++  pk_log(LL, "pk_manager/last_world_update: %llx", 
(uint64_t)pkm->last_world_update);
++  pk_log(LL, "pk_manager/next_tick: %lld", (uint64_t)pkm->next_tick);
+   pk_log(LL, "pk_manager/enable_timer: %d", 0 < pkm->enable_timer);
+   pk_log(LL, "pk_manager/fancy_pagekite_net_rejection_url: %s", 
pkm->fancy_pagekite_net_rejection_url);
+   pk_log(LL, "pk_manager/want_spare_frontends: %d", 
pkm->want_spare_frontends);
+--- a/libpagekite/pkmanager.c
 b/libpagekite/pkmanager.c
+@@ -1070,8 +1070,8 @@ static void pkm_tick_cb(EV_P_ ev_async* 
+ pkm->timer.repeat = pkm->next_tick;
+ ev_timer_again(pkm->loop, &(pkm->timer));
+ pk_log(PK_LOG_MANAGER_INFO,
+-   "Tick!  [repeating=%s, next=%d, status=%d, tunnels=%d, v=%s]",
+-   pkm->enable_timer ? "yes" : "no", pkm->next_tick,
++   "Tick!  [repeating=%s, next=%lld, status=%d, tunnels=%d, v=%s]",
++   pkm->enable_timer ? "yes" : "no", (uint64_t)pkm->next_tick,
+pkm->status, pk_state.live_tunnels, PK_VERSION);
+ 
+ /* We slow down exponentially by default... */
+@@ -1122,8 +1122,8 @@ static void pkm_tick_cb(EV_P_ ev_async* 
+   fe->last_ping = now;
+   pkc_write(&(fe->conn), ping, pingsize);
+   pk_log(PK_LOG_TUNNEL_DATA,
+-  "%d: Sent PING (idle=%ds>%ds)",
+-  fe->conn.sockfd, now - fe->conn.activity, now - inactive);
++  "%d: Sent PING (idle=%llds>%llds)",
++  fe->conn.sockfd, (uint64_t)(now - fe->conn.activity), 
(uint64_t)(now - inactive));
+   next_tick = 1 + pkm->housekeeping_interval_min;
+ }
+   }
+--- a/libpagekite/pkproto.c
 b/libpagekite/pkproto.c
+@@ -577,7 +577,7 @@ int pk_make_salt(char* salt) {
+ char* pk_sign(const char* token, const char* secret, time_t ts,
+   const char* payload, int length, char *buffer)
+ {
+-  char tbuffer[128], tsbuf[16], scratch[10240];
++  char tbuffer[128], tsbuf[32], scratch[10240];
+ 
+   PK_TRACE_FUNCTION;
+ 
+@@ -606,7 +606,7 @@ char* pk_sign(const char* token, const c
+ 
+   /* Optionally embed a timestamp to the resolution of 10 minutes */
+   if (ts > 0) {
+-sprintf(tsbuf, "%lx", ts / 600);
++sprintf(tsbuf, "%llx", (uint64_t)(ts / 600));
+ buffer[0] = 't';
+   }
+   else tsbuf[0] = '\0';
-- 
2.31.1


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

Re: [PATCH] packages: pagekitec: fix crashing due to musl time64

[PATCH] packages: pagekitec: fix crashing due to musl time64

4 matches

Site Navigation

Mail list logo

Footer information