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 <walling@linux.vnet.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Janosch Frank <frankja@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

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 6d0c2ee..9f7904f 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -9,7 +9,7 @@ $(call set-vpath, $(SRC_PATH)/pc-bios/s390-ccw)
 
 .PHONY : all clean build-all
 
-OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o
+OBJECTS = start.o main.o bootmap.o sclp.o virtio.o virtio-scsi.o virtio-blkdev.o libc.o
 QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
 QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
 QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 67a6123..6f8e30f 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -512,7 +512,7 @@ static bool is_iso_bc_entry_compatible(IsoBcSection *s)
                     "Failed to read image sector 0");
 
     /* Checking bytes 8 - 32 for S390 Linux magic */
-    return !_memcmp(magic_sec + 8, linux_s390_magic, 24);
+    return !memcmp(magic_sec + 8, linux_s390_magic, 24);
 }
 
 /* Location of the current sector of the directory */
@@ -641,7 +641,7 @@ static uint32_t find_iso_bc(void)
         if (vd->type == VOL_DESC_TYPE_BOOT) {
             IsoVdElTorito *et = &vd->vd.boot;
 
-            if (!_memcmp(&et->el_torito[0], el_torito_magic, 32)) {
+            if (!memcmp(&et->el_torito[0], el_torito_magic, 32)) {
                 return bswap32(et->bc_offset);
             }
         }
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index cf99a4c..4980838 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -310,20 +310,6 @@ static inline bool magic_match(const void *data, const void *magic)
     return *((uint32_t *)data) == *((uint32_t *)magic);
 }
 
-static inline int _memcmp(const void *s1, const void *s2, size_t n)
-{
-    int i;
-    const uint8_t *p1 = s1, *p2 = s2;
-
-    for (i = 0; i < n; i++) {
-        if (p1[i] != p2[i]) {
-            return p1[i] > p2[i] ? 1 : -1;
-        }
-    }
-
-    return 0;
-}
-
 static inline uint32_t iso_733_to_u32(uint64_t x)
 {
     return (uint32_t)x;
@@ -416,7 +402,7 @@ const uint8_t vol_desc_magic[] = "CD001";
 
 static inline bool is_iso_vd_valid(IsoVolDesc *vd)
 {
-    return !_memcmp(&vd->ident[0], vol_desc_magic, 5) &&
+    return !memcmp(&vd->ident[0], vol_desc_magic, 5) &&
            vd->version == 0x1 &&
            vd->type <= VOL_DESC_TYPE_PARTITION;
 }
diff --git a/pc-bios/s390-ccw/libc.c b/pc-bios/s390-ccw/libc.c
new file mode 100644
index 0000000..60c4b28
--- /dev/null
+++ b/pc-bios/s390-ccw/libc.c
@@ -0,0 +1,75 @@
+/*
+ * libc-style definitions and functions
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include "libc.h"
+
+/**
+ * atoi:
+ * @str: the string to be converted.
+ *
+ * Given a string @str, convert it to an integer. Any non-numerical value
+ * will terminate the conversion.
+ *
+ * Returns: an integer converted from the string @str.
+ */
+int atoi(const char *str)
+{
+    int i;
+    int val = 0;
+
+    for (i = 0; str[i]; i++) {
+        char c = str[i];
+        if (!isdigit(c)) {
+            break;
+        }
+        val *= 10;
+        val += c - '0';
+    }
+
+    return val;
+}
+
+/**
+ * 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;
+    }
+
+    /* Convert int to string */
+    for (i = num_len - 1; i >= 0; i--) {
+        str[i] = num % 10 + '0';
+        num /= 10;
+    }
+
+    str[num_len] = '\0';
+
+    return str;
+}
diff --git a/pc-bios/s390-ccw/libc.h b/pc-bios/s390-ccw/libc.h
index 0142ea8..00268e3 100644
--- a/pc-bios/s390-ccw/libc.h
+++ b/pc-bios/s390-ccw/libc.h
@@ -42,4 +42,35 @@ static inline void *memcpy(void *s1, const void *s2, size_t n)
     return s1;
 }
 
+static inline int memcmp(const void *s1, const void *s2, size_t n)
+{
+    int i;
+    const uint8_t *p1 = s1, *p2 = s2;
+
+    for (i = 0; i < n; i++) {
+        if (p1[i] != p2[i]) {
+            return p1[i] > p2[i] ? 1 : -1;
+        }
+    }
+
+    return 0;
+}
+
+static inline size_t strlen(const char *str)
+{
+    size_t i;
+    for (i = 0; *str; i++) {
+        str++;
+    }
+    return i;
+}
+
+static inline int isdigit(int c)
+{
+    return (c >= '0') && (c <= '9');
+}
+
+int atoi(const char *str);
+char *itostr(int num, char *str, size_t len);
+
 #endif
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 401e9db..a8ef120 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -40,22 +40,7 @@ void panic(const char *string)
 
 unsigned int get_loadparm_index(void)
 {
-    const char *lp = loadparm;
-    int i;
-    unsigned int idx = 0;
-
-    for (i = 0; i < 8; i++) {
-        char c = lp[i];
-
-        if (c < '0' || c > '9') {
-            break;
-        }
-
-        idx *= 10;
-        idx += c - '0';
-    }
-
-    return idx;
+    return atoi(loadparm);
 }
 
 static bool find_dev(Schib *schib, int dev_no)
diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
index b1fc8ff..486fce1 100644
--- a/pc-bios/s390-ccw/sclp.c
+++ b/pc-bios/s390-ccw/sclp.c
@@ -65,14 +65,6 @@ void sclp_setup(void)
     sclp_set_write_mask();
 }
 
-static int _strlen(const char *str)
-{
-    int i;
-    for (i = 0; *str; i++)
-        str++;
-    return i;
-}
-
 long write(int fd, const void *str, size_t len)
 {
     WriteEventData *sccb = (void *)_sccb;
@@ -95,7 +87,7 @@ long write(int fd, const void *str, size_t len)
 
 void sclp_print(const char *str)
 {
-    write(1, str, _strlen(str));
+    write(1, str, strlen(str));
 }
 
 void sclp_get_loadparm_ascii(char *loadparm)
-- 
2.7.4

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 <walling@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Janosch Frank <frankja@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.

> +    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 ?

 Thomas

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 <walling@linux.vnet.ibm.com>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Reviewed-by: Janosch Frank <frankja@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

On 18.12.2017 17:16, Collin L. Walling wrote:
> 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 <walling@linux.vnet.ibm.com>
>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Reviewed-by: Janosch Frank <frankja@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?

Keeping them libc-like likely makes sense ... but could we somehow also
make sure that we're not running into unexpected errors when using them?
Something like "IPL_assert(entries < 1000, ...)" before calling the
functions in patch 5?

 Thomas

On 12/19/2017 02:31 AM, Thomas Huth wrote:
> On 18.12.2017 17:16, Collin L. Walling wrote:
>> 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 <walling@linux.vnet.ibm.com>
>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> Reviewed-by: Janosch Frank <frankja@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?
> Keeping them libc-like likely makes sense ... but could we somehow also
> make sure that we're not running into unexpected errors when using them?
> Something like "IPL_assert(entries < 1000, ...)" before calling the
> functions in patch 5?
>
>   Thomas
>
>

Sounds good to me.

-- 
- Collin L Walling

On 12/19/2017 11:29 AM, Collin L. Walling wrote:
> On 12/19/2017 02:31 AM, Thomas Huth wrote:
>> On 18.12.2017 17:16, Collin L. Walling wrote:
>>> 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 <walling@linux.vnet.ibm.com>
>>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> Reviewed-by: Janosch Frank <frankja@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?
>> Keeping them libc-like likely makes sense ... but could we somehow also
>> make sure that we're not running into unexpected errors when using them?
>> Something like "IPL_assert(entries < 1000, ...)" before calling the
>> functions in patch 5?
>>
>>   Thomas
>>
>>
>
> Sounds good to me.
>


What if we made a wrapper function for itostr. This func will have a tmp 
variable
that stores the return of itostr. We then do an assertion to make sure 
we did not
return 0 (which indicates that the size of the array was not large 
enough). If we
pass, then just return tmp.

e.g.

static char *_itostr(int num, char *str, size_t len)
{
     ...
}

char *itostr(int num, char *str, size_t len)
{
     char *tmp = _itostr(num, str, len);
     IPL_assert(tmp != 0, "array too small for itostr conversion");
     return tmp;
}

And as a side note: we know that the number of boot table entries for 
both ECKD
DASD andSCSI cannot exceed 31, so we should be safe with a rather small 
value
for our char arrays.

-- 
- Collin L Walling

On 19.12.2017 21:23, Collin L. Walling wrote:
> On 12/19/2017 11:29 AM, Collin L. Walling wrote:
>> On 12/19/2017 02:31 AM, Thomas Huth wrote:
>>> On 18.12.2017 17:16, Collin L. Walling wrote:
>>>> 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 <walling@linux.vnet.ibm.com>
>>>>>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> Reviewed-by: Janosch Frank <frankja@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?
>>> Keeping them libc-like likely makes sense ... but could we somehow also
>>> make sure that we're not running into unexpected errors when using them?
>>> Something like "IPL_assert(entries < 1000, ...)" before calling the
>>> functions in patch 5?
>>>
>>>   Thomas
>>>
>>>
>>
>> Sounds good to me.
>>
> 
> 
> What if we made a wrapper function for itostr. This func will have a tmp
> variable
> that stores the return of itostr. We then do an assertion to make sure
> we did not
> return 0 (which indicates that the size of the array was not large
> enough). If we
> pass, then just return tmp.
> 
> e.g.
> 
> static char *_itostr(int num, char *str, size_t len)
> {
>     ...
> }
> 
> char *itostr(int num, char *str, size_t len)
> {
>     char *tmp = _itostr(num, str, len);
>     IPL_assert(tmp != 0, "array too small for itostr conversion");
>     return tmp;
> }

That's fine for me, too.

 Thomas