OCaml fixes for Xen 4.17

[PATCH for-4.17 v3 14/15] tools/ocaml/xenstored/syslog_stubs.c: avoid potential NULL dereference

Posted by Edwin Török 3 years, 3 months ago

If we are out of memory then strdup may return NULL, and passing NULL to
syslog may cause a crash.

Avoid this by using `caml_stat_strdup` which will raise an OCaml out of
memory exception instead.
This then needs to be paired with caml_stat_free.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
Reason for inclusion in 4.17:
- fixes a bug in out of memory situations

Changes since v2:
- new in v3
---
 tools/ocaml/xenstored/syslog_stubs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/xenstored/syslog_stubs.c b/tools/ocaml/xenstored/syslog_stubs.c
index 4e5e49b557..4ad85c8eb5 100644
--- a/tools/ocaml/xenstored/syslog_stubs.c
+++ b/tools/ocaml/xenstored/syslog_stubs.c
@@ -14,6 +14,7 @@
 
 #include <syslog.h>
 #include <string.h>
+#include <caml/fail.h>
 #include <caml/mlvalues.h>
 #include <caml/memory.h>
 #include <caml/alloc.h>
@@ -35,14 +36,16 @@ static int __syslog_facility_table[] = {
 value stub_syslog(value facility, value level, value msg)
 {
     CAMLparam3(facility, level, msg);
-    const char *c_msg = strdup(String_val(msg));
+    char *c_msg = strdup(String_val(msg));
     int c_facility = __syslog_facility_table[Int_val(facility)]
                    | __syslog_level_table[Int_val(level)];
 
+    if ( !c_msg )
+        caml_raise_out_of_memory();
     caml_enter_blocking_section();
     syslog(c_facility, "%s", c_msg);
     caml_leave_blocking_section();
 
-    free((void*)c_msg);
+    free(c_msg);
     CAMLreturn(Val_unit);
 }
-- 
2.34.1

Re: [PATCH for-4.17 v3 14/15] tools/ocaml/xenstored/syslog_stubs.c: avoid potential NULL dereference

Posted by Christian Lindig 3 years, 3 months ago


On 8 Nov 2022, at 15:34, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

If we are out of memory then strdup may return NULL, and passing NULL to
syslog may cause a crash.

Avoid this by using `caml_stat_strdup` which will raise an OCaml out of
memory exception instead.
This then needs to be paired with caml_stat_free.

Signed-off-by: Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>>
---
Reason for inclusion in 4.17:
- fixes a bug in out of memory situations

Changes since v2:
- new in v3
---
tools/ocaml/xenstored/syslog_stubs.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>

Re: [PATCH for-4.17 v3 14/15] tools/ocaml/xenstored/syslog_stubs.c: avoid potential NULL dereference

Posted by Julien Grall 3 years, 3 months ago


On 08/11/2022 15:34, Edwin Török wrote:
> If we are out of memory then strdup may return NULL, and passing NULL to
> syslog may cause a crash.
> 
> Avoid this by using `caml_stat_strdup` which will raise an OCaml out of
> memory exception instead.
> This then needs to be paired with caml_stat_free.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> ---
> Reason for inclusion in 4.17:
> - fixes a bug in out of memory situations
> 
> Changes since v2:
> - new in v3
> ---
>   tools/ocaml/xenstored/syslog_stubs.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/ocaml/xenstored/syslog_stubs.c b/tools/ocaml/xenstored/syslog_stubs.c
> index 4e5e49b557..4ad85c8eb5 100644
> --- a/tools/ocaml/xenstored/syslog_stubs.c
> +++ b/tools/ocaml/xenstored/syslog_stubs.c
> @@ -14,6 +14,7 @@
>   
>   #include <syslog.h>
>   #include <string.h>
> +#include <caml/fail.h>
>   #include <caml/mlvalues.h>
>   #include <caml/memory.h>
>   #include <caml/alloc.h>
> @@ -35,14 +36,16 @@ static int __syslog_facility_table[] = {
>   value stub_syslog(value facility, value level, value msg)
>   {
>       CAMLparam3(facility, level, msg);
> -    const char *c_msg = strdup(String_val(msg));
> +    char *c_msg = strdup(String_val(msg));

This change seems to be unrelated with the goal of the commit. IMHO, 
this should be done in a separate patch.

The minimum would be to mention in the commit message.

Cheers,

-- 
Julien Grall

Re: [PATCH for-4.17 v3 14/15] tools/ocaml/xenstored/syslog_stubs.c: avoid potential NULL dereference

Posted by Edwin Torok 3 years, 3 months ago


> On 8 Nov 2022, at 16:07, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 08/11/2022 15:34, Edwin Török wrote:
>> If we are out of memory then strdup may return NULL, and passing NULL to
>> syslog may cause a crash.
>> Avoid this by using `caml_stat_strdup` which will raise an OCaml out of
>> memory exception instead.
>> This then needs to be paired with caml_stat_free.
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
>> ---
>> Reason for inclusion in 4.17:
>> - fixes a bug in out of memory situations
>> Changes since v2:
>> - new in v3
>> ---
>>  tools/ocaml/xenstored/syslog_stubs.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> diff --git a/tools/ocaml/xenstored/syslog_stubs.c b/tools/ocaml/xenstored/syslog_stubs.c
>> index 4e5e49b557..4ad85c8eb5 100644
>> --- a/tools/ocaml/xenstored/syslog_stubs.c
>> +++ b/tools/ocaml/xenstored/syslog_stubs.c
>> @@ -14,6 +14,7 @@
>>    #include <syslog.h>
>>  #include <string.h>
>> +#include <caml/fail.h>
>>  #include <caml/mlvalues.h>
>>  #include <caml/memory.h>
>>  #include <caml/alloc.h>
>> @@ -35,14 +36,16 @@ static int __syslog_facility_table[] = {
>>  value stub_syslog(value facility, value level, value msg)
>>  {
>>      CAMLparam3(facility, level, msg);
>> -    const char *c_msg = strdup(String_val(msg));
>> +    char *c_msg = strdup(String_val(msg));
> 
> This change seems to be unrelated with the goal of the commit. IMHO, this should be done in a separate patch.
> 
> The minimum would be to mention in the commit message.

That is to avoid freeing 'const char*' (there is a typecast below).
I'll mention it.

> 
> Cheers,
> 
> -- 
> Julien Grall