[Patch 0/4] MdeModulePkg/RegularExpressionDxe: Miss null pointer check

[Patch 0/4] MdeModulePkg/RegularExpressionDxe: Miss null pointer check

[Liming Gao]

Some check are the real issues. Some are for pass static analysis.

Dongao Guo (4):
  MdeModulePkg/RegularExpressionDxe: Miss null pointer check
  MdeModulePkg/RegularExpressionDxe: Miss null pointer check
  MdeModulePkg/RegularExpressionDxe: Miss null pointer check
  MdeModulePkg/RegularExpressionDxe: Add null pointer check

 .../RegularExpressionDxe/Oniguruma/regcomp.c       |  8 +++-
 .../RegularExpressionDxe/Oniguruma/regenc.h        |  2 +-
 .../RegularExpressionDxe/Oniguruma/regexec.c       | 31 ++++++++++++--
 .../RegularExpressionDxe/Oniguruma/regparse.c      | 49 ++++++++++++++++++++--
 .../RegularExpressionDxe/Oniguruma/unicode.c       |  4 ++
 5 files changed, 85 insertions(+), 9 deletions(-)

-- 
2.10.0.windows.1

[Patch 1/4] MdeModulePkg/RegularExpressionDxe: Miss null pointer check

[Liming Gao]

From: Dongao Guo <dongao.guo@intel.com>

Oniguruma https://github.com/kkos/oniguruma
this change is merged from oniguruma develop branch.
from commit 396a757dffafc0c7eb269433c29a0ba961d73ad6.

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
index d274873..6033d21 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
@@ -7725,8 +7725,9 @@ parse_exp(Node** np, OnigToken* tok, int term, UChar** src, UChar* end,
   case TK_ALT:
   case TK_EOT:
   end_of_token:
-  *np = node_new_empty();
-  return tok->type;
+    *np = node_new_empty();
+    CHECK_NULL_RETURN_MEMERR(*np);
+    return tok->type;
   break;
 
   case TK_SUBEXP_OPEN:
@@ -7977,8 +7978,10 @@ parse_exp(Node** np, OnigToken* tok, int term, UChar** src, UChar* end,
     if (IS_SYNTAX_BV(env->syntax, ONIG_SYN_CONTEXT_INDEP_REPEAT_OPS)) {
       if (IS_SYNTAX_BV(env->syntax, ONIG_SYN_CONTEXT_INVALID_REPEAT_OPS))
         return ONIGERR_TARGET_OF_REPEAT_OPERATOR_NOT_SPECIFIED;
-      else
+      else {
         *np = node_new_empty();
+        CHECK_NULL_RETURN_MEMERR(*np);
+      }
     }
     else {
       goto tk_byte;
-- 
2.10.0.windows.1

[Patch 2/4] MdeModulePkg/RegularExpressionDxe: Miss null pointer check

[Liming Gao]

From: Dongao Guo <dongao.guo@intel.com>

Oniguruma https://github.com/kkos/oniguruma
this change is merged from oniguruma develop branch.
from commit ea36d810f1d9b28f3ef20bd8d453bea2f7fb598b

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
---
 .../RegularExpressionDxe/Oniguruma/regenc.h        |  2 +-
 .../RegularExpressionDxe/Oniguruma/regparse.c      | 34 ++++++++++++++++++++++
 .../RegularExpressionDxe/Oniguruma/unicode.c       |  4 +++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.h b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.h
index 6235520..46a5142 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.h
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regenc.h
@@ -197,7 +197,7 @@ extern int onigenc_egcb_is_break_position P_((OnigEncoding enc, UChar* p, UChar*
   else if ((buk)->fold_len == 3)\
     addr = OnigUnicodeFolds3 + (buk)->index;\
   else\
-    addr = 0;\
+    return ONIGERR_INVALID_CODE_POINT_VALUE;\
 } while (0)
 
 extern OnigCodePoint OnigUnicodeFolds1[];
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
index 6033d21..5b7fec9 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
@@ -966,6 +966,7 @@ name_add(regex_t* reg, UChar* name, UChar* name_end, int backref, ScanEnv* env)
 #ifdef USE_ST_LIBRARY
     if (IS_NULL(t)) {
       t = onig_st_init_strend_table_with_size(INIT_NAMES_ALLOC_NUM);
+      CHECK_NULL_RETURN_MEMERR(t);
       reg->name_table = (void* )t;
     }
     e = (NameEntry* )xmalloc(sizeof(NameEntry));
@@ -1372,6 +1373,7 @@ callout_name_entry(CalloutNameEntry** rentry, OnigEncoding enc,
 #ifdef USE_ST_LIBRARY
     if (IS_NULL(t)) {
       t = onig_st_init_callout_name_table_with_size(INIT_NAMES_ALLOC_NUM);
+      CHECK_NULL_RETURN_MEMERR(t);
       GlobalCalloutNameTable = t;
     }
     e = (CalloutNameEntry* )xmalloc(sizeof(CalloutNameEntry));
@@ -1616,6 +1618,7 @@ onig_get_callout_start_func(regex_t* reg, int callout_num)
   CalloutListEntry* e;
 
   e = onig_reg_callout_list_at(reg, callout_num);
+  CHECK_NULL_RETURN(e);
   return e->start_func;
 }
 
@@ -1623,6 +1626,7 @@ extern const UChar*
 onig_get_callout_tag_start(regex_t* reg, int callout_num)
 {
   CalloutListEntry* e = onig_reg_callout_list_at(reg, callout_num);
+  CHECK_NULL_RETURN(e);
   return e->tag_start;
 }
 
@@ -1630,6 +1634,7 @@ extern const UChar*
 onig_get_callout_tag_end(regex_t* reg, int callout_num)
 {
   CalloutListEntry* e = onig_reg_callout_list_at(reg, callout_num);
+  CHECK_NULL_RETURN(e);
   return e->tag_end;
 }
 
@@ -1904,6 +1909,7 @@ callout_tag_entry(regex_t* reg, UChar* name, UChar* name_end,
   r = callout_tag_entry_raw(ext->tag_table, name, name_end, entry_val);
 
   e = onig_reg_callout_list_at(reg, (int )entry_val);
+  CHECK_NULL_RETURN_MEMERR(e);
   e->tag_start = name;
   e->tag_end   = name_end;
 
@@ -2138,6 +2144,8 @@ node_new_anychar_with_fixed_option(OnigOptionType option)
   Node* node;
 
   node = node_new_anychar();
+  CHECK_NULL_RETURN(node);
+
   ct = CTYPE_(node);
   ct->options = option;
   NODE_STATUS_ADD(node, FIXED_OPTION);
@@ -3174,6 +3182,7 @@ static Node*
 node_new_str_raw(UChar* s, UChar* end)
 {
   Node* node = node_new_str(s, end);
+  CHECK_NULL_RETURN(node);
   NODE_STRING_SET_RAW(node);
   return node;
 }
@@ -3206,6 +3215,7 @@ str_node_split_last_char(Node* node, OnigEncoding enc)
     p = onigenc_get_prev_char_head(enc, sn->s, sn->end);
     if (p && p > sn->s) { /* can be split. */
       rn = node_new_str(p, sn->end);
+      CHECK_NULL_RETURN(rn);
       if (NODE_STRING_IS_RAW(node))
         NODE_STRING_SET_RAW(rn);
 
@@ -6626,6 +6636,11 @@ parse_callout_of_contents(Node** np, int cterm, UChar** src, UChar* end, ScanEnv
   }
 
   e = onig_reg_callout_list_at(env->reg, num);
+  if (IS_NULL(e)) {
+    xfree(contents);
+    return ONIGERR_MEMORY;
+  }
+
   e->of      = ONIG_CALLOUT_OF_CONTENTS;
   e->in      = in;
   e->name_id = ONIG_NON_NAME_ID;
@@ -6935,6 +6950,8 @@ parse_callout_of_name(Node** np, int cterm, UChar** src, UChar* end, ScanEnv* en
   if (r != ONIG_NORMAL) return r;
 
   e = onig_reg_callout_list_at(env->reg, num);
+  CHECK_NULL_RETURN_MEMERR(e);
+
   e->of         = ONIG_CALLOUT_OF_NAME;
   e->in         = in;
   e->name_id    = name_id;
@@ -8098,6 +8115,11 @@ parse_branch(Node** top, OnigToken* tok, int term, UChar** src, UChar* end,
   }
   else {
     *top  = node_new_list(node, NULL);
+    if (IS_NULL(*top)) {
+      onig_node_free(node);
+      return ONIGERR_MEMORY;
+    }
+
     headp = &(NODE_CDR(*top));
     while (r != TK_EOT && r != term && r != TK_ALT) {
       r = parse_exp(&node, tok, term, src, end, env);
@@ -8133,6 +8155,7 @@ parse_subexp(Node** top, OnigToken* tok, int term, UChar** src, UChar* end,
   env->parse_depth++;
   if (env->parse_depth > ParseDepthLimit)
     return ONIGERR_PARSE_DEPTH_LIMIT_OVER;
+
   r = parse_branch(&node, tok, term, src, end, env);
   if (r < 0) {
     onig_node_free(node);
@@ -8144,6 +8167,11 @@ parse_subexp(Node** top, OnigToken* tok, int term, UChar** src, UChar* end,
   }
   else if (r == TK_ALT) {
     *top  = onig_node_new_alt(node, NULL);
+    if (IS_NULL(*top)) {
+      onig_node_free(node);
+      return ONIGERR_MEMORY;
+    }
+
     headp = &(NODE_CDR(*top));
     while (r == TK_ALT) {
       r = fetch_token(tok, src, end, env);
@@ -8154,6 +8182,12 @@ parse_subexp(Node** top, OnigToken* tok, int term, UChar** src, UChar* end,
         return r;
       }
       *headp = onig_node_new_alt(node, NULL);
+      if (IS_NULL(*headp)) {
+        onig_node_free(node);
+        onig_node_free(*top);
+        return ONIGERR_MEMORY;
+      }
+
       headp = &(NODE_CDR(*headp));
     }
 
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/unicode.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/unicode.c
index 1587500..16c34b6 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/unicode.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/unicode.c
@@ -758,6 +758,10 @@ onig_unicode_define_user_property(const char* name, OnigCodePoint* ranges)
 
   if (UserDefinedPropertyTable == 0) {
     UserDefinedPropertyTable = onig_st_init_strend_table_with_size(10);
+    if (IS_NULL(UserDefinedPropertyTable)) {
+      xfree(s);
+      return ONIGERR_MEMORY;
+    }
   }
 
   e = UserDefinedPropertyRanges + UserDefinedPropertyNum;
-- 
2.10.0.windows.1

[Patch 3/4] MdeModulePkg/RegularExpressionDxe: Miss null pointer check

[Liming Gao]

From: Dongao Guo <dongao.guo@intel.com>

Oniguruma https://github.com/kkos/oniguruma
this change is merged from oniguruma develop branch.
from commit 1db8a2726dfad0401f928cb8474bd770f07040a7.

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
---
 MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
index 5b7fec9..c71ae5f 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
@@ -1906,6 +1906,7 @@ callout_tag_entry(regex_t* reg, UChar* name, UChar* name_end,
   if (r != ONIG_NORMAL) return r;
 
   ext = onig_get_regex_ext(reg);
+  CHECK_NULL_RETURN_MEMERR(ext);
   r = callout_tag_entry_raw(ext->tag_table, name, name_end, entry_val);
 
   e = onig_reg_callout_list_at(reg, (int )entry_val);
@@ -2088,6 +2089,7 @@ node_new(void)
   Node* node;
 
   node = (Node* )xmalloc(sizeof(Node));
+  CHECK_NULL_RETURN(node);
   xmemset(node, 0, sizeof(*node));
 
 #ifdef DEBUG_NODE_FREE
@@ -6616,6 +6618,7 @@ parse_callout_of_contents(Node** np, int cterm, UChar** src, UChar* end, ScanEnv
   if (r != 0) return r;
 
   ext = onig_get_regex_ext(env->reg);
+  CHECK_NULL_RETURN_MEMERR(ext);
   if (IS_NULL(ext->pattern)) {
     r = onig_ext_set_pattern(env->reg, env->pattern, env->pattern_end);
     if (r != ONIG_NORMAL) return r;
@@ -6936,6 +6939,7 @@ parse_callout_of_name(Node** np, int cterm, UChar** src, UChar* end, ScanEnv* en
   if (r != 0) return r;
 
   ext = onig_get_regex_ext(env->reg);
+  CHECK_NULL_RETURN_MEMERR(ext);
   if (IS_NULL(ext->pattern)) {
     r = onig_ext_set_pattern(env->reg, env->pattern, env->pattern_end);
     if (r != ONIG_NORMAL) return r;
@@ -7987,6 +7991,7 @@ parse_exp(Node** np, OnigToken* tok, int term, UChar** src, UChar* end,
       int ascii_mode =
         IS_WORD_ASCII(env->options) && IS_WORD_ANCHOR_TYPE(tok->u.anchor) ? 1 : 0;
       *np = onig_node_new_anchor(tok->u.anchor, ascii_mode);
+	  CHECK_NULL_RETURN_MEMERR(*np);
     }
     break;
 
-- 
2.10.0.windows.1

[Patch 4/4] MdeModulePkg/RegularExpressionDxe: Add null pointer check

[Liming Gao]

From: Dongao Guo <dongao.guo@intel.com>

There are five check not necessary in logic ,just for pass static
analysis. More detail please refer to comment in code.
And the rest changes are all accepted by owner, they are reasonable.

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Dongao Guo <dongao.guo@intel.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>
---
 .../RegularExpressionDxe/Oniguruma/regcomp.c       |  8 ++++--
 .../RegularExpressionDxe/Oniguruma/regexec.c       | 31 +++++++++++++++++++---
 .../RegularExpressionDxe/Oniguruma/regparse.c      |  1 +
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
index cc06178..5cefafc 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regcomp.c
@@ -3546,8 +3546,12 @@ expand_case_fold_string_alt(int item_num, OnigCaseFoldCodeItem items[], UChar *p
     if (IS_NULL(an)) {
       goto mem_err2;
     }
-
-    if (items[i].byte_len != slen) {
+    //The NULL pointer check is not necessary. It is added just for pass static
+    //analysis. When condition "items[i].byte_len != slen" is true, "varlen = 1" 
+    //in line 3503 will be reached ,so that "if (IS_NULL(var_anode)) return ONIGERR_MEMORY" 
+    //in line 3510 will be executed, so the null pointer has been checked before 
+    //deferenced in line 3584.
+    if (items[i].byte_len != slen && IS_NOT_NULL(var_anode)) {
       Node *rem;
       UChar *q = p + items[i].byte_len;
 
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
index 26e7a31..7f5bb8b 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regexec.c
@@ -4088,6 +4088,11 @@ slow_search_backward(OnigEncoding enc, UChar* target, UChar* target_end,
     s = ONIGENC_LEFT_ADJUST_CHAR_HEAD(enc, adjust_text, s);
 
   while (s >= text) {
+    //if text is not null,the logic is correct.
+    //this function is only invoked by backward_search_range,parameter text come 
+    //from range, which is checked by "if (range == 0) goto fail" in line 4512
+    //so the check is just for passing static analysis.
+    if(IS_NULL(s))break;
     if (*s == *target) {
       p = s + 1;
       t = target + 1;
@@ -4298,6 +4303,11 @@ map_search_backward(OnigEncoding enc, UChar map[],
   const UChar *s = text_start;
 
   while (s >= text) {
+    //if text is not null,the logic is correct.
+    //this function is only invoked by backward_search_range,parameter text come 
+    //from range, which is checked by "if (range == 0) goto fail" in line 4512
+    //so the check is just for passing static analysis.
+    if(IS_NULL(s))break;
     if (map[*s]) return (UChar* )s;
 
     s = onigenc_get_prev_char_head(enc, adjust_text, s);
@@ -4499,7 +4509,7 @@ backward_search_range(regex_t* reg, const UChar* str, const UChar* end,
                       UChar** low, UChar** high)
 {
   UChar *p;
-
+  if (range == 0) goto fail;
   range += reg->dmin;
   p = s;
 
@@ -4550,7 +4560,7 @@ backward_search_range(regex_t* reg, const UChar* str, const UChar* end,
       case ANCHOR_BEGIN_LINE:
         if (!ON_STR_BEGIN(p)) {
           prev = onigenc_get_prev_char_head(reg->enc, str, p);
-          if (!ONIGENC_IS_MBC_NEWLINE(reg->enc, prev, end)) {
+          if (IS_NOT_NULL(prev) && !ONIGENC_IS_MBC_NEWLINE(reg->enc, prev, end)) {
             p = prev;
             goto retry;
           }
@@ -4739,10 +4749,15 @@ onig_search_with_param(regex_t* reg, const UChar* str, const UChar* end,
       }
     }
     else if (reg->anchor & ANCHOR_SEMI_END_BUF) {
+
       UChar* pre_end = ONIGENC_STEP_BACK(reg->enc, str, end, 1);
 
       max_semi_end = (UChar* )end;
-      if (ONIGENC_IS_MBC_NEWLINE(reg->enc, pre_end, end)) {
+      // only when str > end, pre_end will be null
+      // line 4659 "if (start > end || start < str) goto mismatch_no_msa" 
+      // will guarantee str alwayls less than end
+      // so pre_end won't be null,this check is just for passing staic analysis
+      if (IS_NOT_NULL(pre_end) && ONIGENC_IS_MBC_NEWLINE(reg->enc, pre_end, end)) {
         min_semi_end = pre_end;
 
 #ifdef USE_CRNL_AS_LINE_TERMINATOR
@@ -4891,6 +4906,16 @@ onig_search_with_param(regex_t* reg, const UChar* str, const UChar* end,
             MATCH_AND_RETURN_CHECK(orig_start);
             s = prev;
           }
+          // if range is not null,the check is not necessary.
+          // the range is actually the pointer of the end of the matched string 
+          // or assigned by "range = str" in line 4708. In RegularExpressionMatch 
+          // protocol, the matched string is the parameter String. And str in 
+          // line 4708 is the String,too. and the range is calculated from 
+          // "Start + onigenc_str_bytelen_null (CHAR16_ENCODING, Start)" in 
+          // line 146 in RegularExpressionDxe.c. RegularExpressionMatch ensure 
+          // the String is not null,So in both situation, the range can not be NULL.
+          // This check is just for passing static analysis.
+          if(IS_NULL(s))break;
         } while (s >= range);
         goto mismatch;
       }
diff --git a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
index c71ae5f..0a2d6f8 100644
--- a/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
+++ b/MdeModulePkg/Universal/RegularExpressionDxe/Oniguruma/regparse.c
@@ -1572,6 +1572,7 @@ onig_set_callout_of_name(OnigEncoding enc, OnigCalloutType callout_type,
     fe->arg_types[i] = arg_types[i];
   }
   for (i = arg_num - opt_arg_num, j = 0; i < arg_num; i++, j++) {
+    if(IS_NULL(opt_defaults))return ONIGERR_INVALID_ARGUMENT;
     if (fe->arg_types[i] == ONIG_TYPE_STRING) {
       OnigValue* val = opt_defaults + j;
       UChar* ds = onigenc_strdup(enc, val->s.start, val->s.end);
-- 
2.10.0.windows.1