<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hello,<br>
</p>
<div class="moz-cite-prefix">On 20.09.19 11:27, Henning Westerholt
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:b3b8ca40-45f0-228b-9c06-08ce014b0eb9@skalatan.de">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hello Daniel,</p>
<p>thanks for the notice of the buffer length - will fix it and
test again.</p>
<p>The report from the tool mentioned that it is an issue also for
multi-process, but of course more critical for multi-threaded:<br>
</p>
<p>"The time related functions such as <code>gmtime</code> fill
data into a <code>
tm</code> struct or <code>char</code> array in shared memory
and then returns a pointer to that memory. If the function is
called from multiple places in the same program, and especially
if it is called from multiple threads in the same program, then
the calls will overwrite each other's data."</p>
<p>Therefore I think that it is definitely safer, especially as
there might be libraries that use it as well, that we don't see.</p>
</blockquote>
<p><br>
</p>
<p>When is single process, not multi-threading, there can be an
issue when keeping a global variable with the result of ctime()
(and the other functions) and using it later expecting the same
result, while in between the executed code could have been used
the same function.</p>
<p>But in the functions you did the change, there were local
operations within the scope of the functions and the result of the
time-related functions was copied. There is no other function from
other library executed that could overwrite the value.</p>
<p>Anyhow, in the current Kamailio architecture is not making
anything safer compared with the old variant. Maybe for the future
were multi-threading is added to some extent. Overall I am fine
with the change, just wanted to be sure was like a cosmetic
change, not something that created problems.</p>
<p>Cheers,<br>
Daniel<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:b3b8ca40-45f0-228b-9c06-08ce014b0eb9@skalatan.de">
<p>We probably don't need to backport it, thought.<br>
</p>
<p>Cheers,</p>
<p>Henning<br>
</p>
<div class="moz-cite-prefix">Am 20.09.19 um 11:03 schrieb
Daniel-Constantin Mierla:<br>
</div>
<blockquote type="cite"
cite="mid:6f6012f8-123d-b186-07f3-0c366e3cc35b@gmail.com">
<pre class="moz-quote-pre" wrap="">Hello,
were these changes to use ctime_r() (and in other commits gmtime_r(),
localtime_r(), asctime_r()) required to fix existing issues or more like
a preference to use them? Because Kamailio is multiprocess and thread
safety should not be a concern, otherwise there might be a lot of other
place to take care of thread safety...
Anyhow, there are out of bound writes introduced as the buffers must be
at least 26 chars long, not 25 -- those need to be fixed.
Cheers,
Daniel
On 20.09.19 00:04, Henning Westerholt wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Module: kamailio
Branch: master
Commit: dc2acb895538131e99c770da6f7448cb5a46fc32
URL: <a class="moz-txt-link-freetext" href="https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32" moz-do-not-send="true">https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32</a>
Author: Henning Westerholt <a class="moz-txt-link-rfc2396E" href="mailto:hw@skalatan.de" moz-do-not-send="true"><hw@skalatan.de></a>
Committer: Henning Westerholt <a class="moz-txt-link-rfc2396E" href="mailto:hw@skalatan.de" moz-do-not-send="true"><hw@skalatan.de></a>
Date: 2019-09-19T23:49:32+02:00
core: replace glibc time function calls with the thread-safe versions
- replace glibc time function calls with the thread-safe versions, to prevent
race conditions from multi-process / multi-threaded access
- used in 'kamcmd core.uptime' rpc cmd, no functional change, locally tested
---
Modified: src/core/core_cmd.c
---
Diff: <a class="moz-txt-link-freetext" href="https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32.diff" moz-do-not-send="true">https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32.diff</a>
Patch: <a class="moz-txt-link-freetext" href="https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32.patch" moz-do-not-send="true">https://github.com/kamailio/kamailio/commit/dc2acb895538131e99c770da6f7448cb5a46fc32.patch</a>
---
diff --git a/src/core/core_cmd.c b/src/core/core_cmd.c
index 5b1c4624ed..717e240fde 100644
--- a/src/core/core_cmd.c
+++ b/src/core/core_cmd.c
@@ -224,8 +224,7 @@ static const char* dst_blst_stats_get_doc[] = {
#endif
-
-#define MAX_CTIME_LEN 128
+#define MAX_CTIME_LEN 25
/* up time */
static char up_since_ctime[MAX_CTIME_LEN];
@@ -381,13 +380,14 @@ static void core_uptime(rpc_t* rpc, void* c)
{
void* s;
time_t now;
+ char buf[MAX_CTIME_LEN];
str snow;
+ snow.s = buf;
time(&now);
if (rpc->add(c, "{", &s) < 0) return;
- snow.s = ctime(&now);
- if(snow.s) {
+ if(ctime_r(&now, snow.s)) {
snow.len = strlen(snow.s);
if(snow.len>2 && snow.s[snow.len-1]=='\n') snow.len--;
rpc->struct_add(s, "S", "now", &snow);
@@ -1187,21 +1187,14 @@ int register_core_rpcs(void)
int rpc_init_time(void)
{
- char *t;
- t=ctime(&up_since);
- if (strlen(t)+1>=MAX_CTIME_LEN) {
- ERR("Too long data %d\n", (int)strlen(t));
+ char t[MAX_CTIME_LEN];
+ int len;
+ if (! ctime_r(&up_since, t)) {
+ ERR("Invalid time value\n");
return -1;
}
strcpy(up_since_ctime, t);
- t = up_since_ctime + strlen(up_since_ctime);
- while(t>up_since_ctime) {
- if(*t=='\0' || *t=='\r' || *t=='\n') {
- *t = '\0';
- } else {
- break;
- }
- t--;
- }
+ len = strlen(up_since_ctime);
+ if(len>2 && up_since_ctime[len-1]=='\n') up_since_ctime[len-1]='\0';
return 0;
}
_______________________________________________
Kamailio (SER) - Development Mailing List
<a class="moz-txt-link-abbreviated" href="mailto:sr-dev@lists.kamailio.org" moz-do-not-send="true">sr-dev@lists.kamailio.org</a>
<a class="moz-txt-link-freetext" href="https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev" moz-do-not-send="true">https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev</a>
</pre>
</blockquote>
</blockquote>
<pre class="moz-signature" cols="72">--
Kamailio Merchandising - <a class="moz-txt-link-freetext" href="https://skalatan.de/merchandising/" moz-do-not-send="true">https://skalatan.de/merchandising/</a>
Kamailio services - <a class="moz-txt-link-freetext" href="https://skalatan.de/services" moz-do-not-send="true">https://skalatan.de/services</a>
Henning Westerholt - <a class="moz-txt-link-freetext" href="https://skalatan.de/blog/" moz-do-not-send="true">https://skalatan.de/blog/</a></pre>
</blockquote>
<pre class="moz-signature" cols="72">--
Daniel-Constantin Mierla -- <a class="moz-txt-link-abbreviated" href="http://www.asipto.com">www.asipto.com</a>
<a class="moz-txt-link-abbreviated" href="http://www.twitter.com/miconda">www.twitter.com/miconda</a> -- <a class="moz-txt-link-abbreviated" href="http://www.linkedin.com/in/miconda">www.linkedin.com/in/miconda</a>
Kamailio Advanced Training, Oct 21-23, 2019, Berlin, Germany -- <a class="moz-txt-link-freetext" href="https://asipto.com/u/kat">https://asipto.com/u/kat</a></pre>
</body>
</html>