Hi Magnus,
Thank you for reporting this problem and providing a patch. Most
places that use strftime already test for a return value of zero, but
I see two that do not. Perhaps using a macro that can be used from
multiple places will be a better solution than changing the code in
various places. See attached variation of your patch.
Moe
Quoting Magnus Jonsson <[email protected]>:
Hi!
We found an issue in sacct that we pined down to a strftime call in
'src/common/parse_time.c' (slurm_make_time_str).
Reproducable with (in 2.5.{6,7}):
OK:
% sacct -Po end%19 -s failed,completed,timeout,cancelled -S
2013-06-13 | head | cat -vet
2013-06-14T11:40:14$
2013-06-14T11:40:14$
2013-06-14T11:40:11$
Fail:
sacct -Po end%18 -s failed,completed,timeout,cancelled -S 2013-06-13
| head | cat -vet
End$
2013-06-14TVM-^P^?$
2013-06-14TVM-^P^?$
2013-06-14TVM-^P^?$
The problem is that the output from strftime with a buffer less the
the required length of output is undefined in libc since libc 4.4.4.
Different libc implementations seams to implement this differently.
On Solaris for example the buffer is truncated at the given length
but still returns 0.
From man page of strftime (Ubuntu Precise):
------8<------------
RETURN VALUE
The strftime() function returns the number of characters placed in the
array s, not including the terminating null byte, provided the string,
including the terminating null byte, fits. Otherwise, it returns 0,
and the contents of the array is undefined. (This behavior applies
since at least libc 4.4.4; very old versions of libc, such as libc
4.4.1, would return max if the array was too small.)
Note that the return value 0 does not necessarily indicate an error;
for example, in many locales %p yields an empty string.
------8<------------
From man page of strftime (Solaris)
------8<------------
If the total number of resulting characters including the
terminating null character is more than maxsize, strftime() returns
0 and the contents of the array are indeterminate.
------8<------------
The return value of strftime is not checked for in
slurm_make_time_str making the returned value undefined.
As I see it the problem can be solved in many ways.
1. using a "large" temporary buffer for the output the expected
behaviour of sacct end%N will for most normal cases be fine.
2. Only checking the return value and returning an error or an well
defined output.
I haved attached a patch for case 1 with that sets the output to "#"
if the output still not fit into the buffer.
There seams to be other places in the slurm code base that uses
strftime and not checking the return code. Some of them might be OK
due to the format string and size of the buffer. But this might
needs to be looked into with more depth.
Best regards,
Magnus
--
Magnus Jonsson, Developer, HPC2N, Umeå Universitet
diff --git a/src/common/macros.h b/src/common/macros.h
index 5124a61..f8c1e90 100644
--- a/src/common/macros.h
+++ b/src/common/macros.h
@@ -275,6 +275,19 @@ typedef enum {false, true} bool;
# define strndup(src,size) strdup(src)
#endif
+/* Results strftime() are undefined if buffer too small
+ * This variant returns a string of "####"... instead */
+#define slurm_strftime(s, max, format, tm) \
+_STMT_START { \
+ if (max > 0) { \
+ char tmp_string[(max<256?256:max+1)]; \
+ if (strftime(tmp_string, sizeof(tmp_string), format, tm) == 0) \
+ memset(tmp_string, '#', max); \
+ tmp_string[max-1] = 0; \
+ strncpy(s, tmp_string, max); \
+ } \
+} _STMT_END
+
/* There are places where we put NO_VAL or INFINITE into a float or double
* Use fuzzy_equal below to test for those values rather than an comparision
* which could fail due to rounding errors. */
diff --git a/src/common/parse_time.c b/src/common/parse_time.c
index f5536ad..f795a31 100644
--- a/src/common/parse_time.c
+++ b/src/common/parse_time.c
@@ -626,7 +626,7 @@ slurm_make_time_str (time_t *time, char *string, int size)
if (use_relative_format)
display_fmt = _relative_date_fmt(&time_tm);
- strftime(string, size, display_fmt, &time_tm);
+ slurm_strftime(string, size, display_fmt, &time_tm);
}
}
diff --git a/src/common/slurm_cred.c b/src/common/slurm_cred.c
index 1c41355..1f22156 100644
--- a/src/common/slurm_cred.c
+++ b/src/common/slurm_cred.c
@@ -1684,7 +1684,7 @@ extern char * timestr (const time_t *tp, char *buf, size_t n)
#endif
if (!localtime_r (tp, &tmval))
error ("localtime_r: %m");
- strftime (buf, n, fmt, &tmval);
+ slurm_strftime (buf, n, fmt, &tmval);
return (buf);
}
