On 12/18/2017 08:06 AM, Thomas Huth wrote:
On 11.12.2017 23:19, Collin L. Walling wrote:
Moved:
memcmp from bootmap.h to libc.h (renamed from _memcmp)
strlen from sclp.c to libc.h (renamed from _strlen)
Added C standard functions:
isdigit
atoi
Added non-C standard function:
itostr
Signed-off-by: Collin L. Walling <wall...@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntrae...@de.ibm.com>
Reviewed-by: Janosch Frank <fran...@linux.vnet.ibm.com>
---
pc-bios/s390-ccw/Makefile | 2 +-
pc-bios/s390-ccw/bootmap.c | 4 +--
pc-bios/s390-ccw/bootmap.h | 16 +---------
pc-bios/s390-ccw/libc.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/libc.h | 31 +++++++++++++++++++
pc-bios/s390-ccw/main.c | 17 +----------
pc-bios/s390-ccw/sclp.c | 10 +------
7 files changed, 112 insertions(+), 43 deletions(-)
create mode 100644 pc-bios/s390-ccw/libc.c
[...]
+
+/**
+ * itostr:
+ * @num: the integer to be converted.
+ * @str: a pointer to a string to store the conversion.
+ * @len: the length of the passed string.
+ *
+ * Given an integer @num, convert it to a string. The string @str must be
+ * allocated beforehand. The resulting string will be null terminated and
+ * returned.
+ *
+ * Returns: the string @str of the converted integer @num.
+ */
+char *itostr(int num, char *str, size_t len)
+{
+ long num_len = 1;
+ int tmp = num;
+ int i;
+
+ /* Count length of num */
+ while ((tmp /= 10) > 0) {
+ num_len++;
+ }
+
+ /* Check if we have enough space for num and null */
+ if (len < num_len) {
+ return 0;
+ }
I'm afraid, but I think you've got an off-by-one bug in this code.
In patch 5, you're using this function like this:
char tmp[4];
sclp_print(itostr(entries, tmp, sizeof(tmp)));
That means if entries is >= 1000 for example, num_len is 4 ...
+ /* Convert int to string */
+ for (i = num_len - 1; i >= 0; i--) {
+ str[i] = num % 10 + '0';
+ num /= 10;
+ }
+
+ str[num_len] = '\0';
... and then you run into a buffer overflow here.
Doh, you're correct. I forgot to put a "<=" in the len / num_len check.
That should fix things up. Thanks for catching that.
+ return str;
+}
Maybe it would also make more sense to panic() instead of "return 0"
since you don't check the return value in patch 5 ?
I'm a bit conflicted about doing something like that. I'm not sure if
there's any kind
of guideline we want to follow for defining functions in libc.
I see one of two possibilities:
a. define these functions as "libc-like" as possible, and use them as
if they were
regular standard libc functions
or
b. change up these functions to better fit their use cases in
pc-bios/s390-ccw
Does that make sense? What do you think?
Thomas
--
- Collin L Walling
