[PATCH] D99393: [OCaml] Fix a possible crash in llvm_struct_name

[Vaivaswatha Nagaraj via Phabricator via llvm-commits]

vaivaswatha added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:887
   unsigned Len;
-
-  if ((S = LLVMGetMDString(V, &Len))) {
-    Str = caml_alloc_string(Len);
-    memcpy((char *)String_val(Str), S, Len);
-    Option = alloc(1,0);
-    Store_field(Option, 0, Str);
-    CAMLreturn(Option);
-  }
-  CAMLreturn(Val_int(0));
+  return cstr_to_string_option(LLVMGetMDString(V, &Len), Len);
 }
----------------
Given that the order of argument evaluation is unspecified in C, if the second argument `Len` to `cstr_to_string_option` is evaluated first (at which point it is uninitialized), then an incorrect value gets passed. 


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1045
   if(LLVMIsAConstantDataSequential(Const) && LLVMIsConstantString(Const)) {
-    S = LLVMGetAsString(Const, &Len);
-    Str = caml_alloc_string(Len);
-    memcpy((char *)String_val(Str), S, Len);
-
-    Option = alloc(1, 0);
-    Field(Option, 0) = Str;
-    CAMLreturn(Option);
+    return cstr_to_string_option(LLVMGetAsString(Const, &Len), Len);
   } else {
----------------
Same as the previous change.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1047
   } else {
-    CAMLreturn(Val_int(0));
+    return Val_int(0);
   }
----------------
Would it be a good idea to define a macro `#define OCAML_OPTION_NONE Val_Int(0)` to use in such places? I'm not saying we should do it in this patch (if you think it's a good idea to do it at all), but may be in a later one that changes it at all places uniformly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99393/new/

https://reviews.llvm.org/D99393