Possible wrong fix to #207

[voxik]

I'm still trying to wrap my head around cbe9f8b. The question is whether s is NULL terminated string:

oniguruma/src/regcomp.c

Lines 6030 to 6036 in cbdbdad

	typedef struct {
	MinMaxLen mm; /* position */
	OptAnc anc;
	int reach_end;
	int len;
	UChar s[OPT_EXACT_MAXLEN];
	} OptStr;

My conclusion is that it is just array of characters, because it is accompanied by len field, and looking at the other places, nothing suggest it should be NULL terminated. The only place, where it could appear NULL terminated is:

oniguruma/src/regcomp.c

Lines 6213 to 6221 in cbdbdad

	static void
	clear_opt_exact(OptStr* e)
	{
	mml_clear(&e->mm);
	clear_opt_anc_info(&e->anc);
	e->reach_end = 0;
	e->len = 0;
	e->s[0] = '\0';
	}

but that is just clearing the struct.

So given the s is just array of OPT_EXACT_MAXLEN characters, lets debug through concat_opt_exact_str:

oniguruma/src/regcomp.c

Lines 6259 to 6276 in cbdbdad

	static void
	concat_opt_exact_str(OptStr* to, UChar* s, UChar* end, OnigEncoding enc)
	{
	int i, j, len;
	UChar *p;
	for (i = to->len, p = s; p < end && i < OPT_EXACT_MAXLEN; ) {
	len = enclen(enc, p);
	if (i + len >= OPT_EXACT_MAXLEN) break;
	for (j = 0; j < len && p < end; j++)
	to->s[i++] = *p++;
	}
	to->len = i;
	if (p >= end)
	to->reach_end = 1;
	}

and being on line 6267, I have following values:

(gdb) print s
$15 = (OnigUChar *) 0x4054c0 "a"
(gdb) print end
$16 = (OnigUChar *) 0x4054c1 ""

(gdb) print *(OptStr *)to
$25 = {mmd = {min = 0, max = 0}, anc = {left_anchor = 0, right_anchor = 0}, reach_end = 0, len = 0, 
  s = "\000\000\000\000H\334\377\377\377\177\000\000T\334\377\377\377\177\000\000\000\000\000"}

(gdb) print len
$29 = 1

That in turn means that i + len == 0 + 1 == 1. If OPT_EXACT_MAXLEN was 1, then the condition if (i + len >= OPT_EXACT_MAXLEN) break; would turn into if (1 >= 1) break;, which would break, although there is certainly one byte available in the output string.

This tends me believe, that original condition, i.e. if (i + len > OPT_EXACT_MAXLEN) break; was the correct condition and the cbe9f8b should be reverted.

This possible applies also to 8155473, but I have not checked the case.

[voxik]

BTW if my analysis is wrong, then I would appreciate, if there is added test case covering this function and documentation for the OptStr was extended a bit.

[rofl0r]

and the cbe9f8b should be reverted.

This possible applies also to cbe9f8b

i suppose the second commit hash is a copy paste error ?

[voxik]

and the cbe9f8b should be reverted.

This possible applies also to cbe9f8b

i suppose the second commit hash is a copy paste error ?

Ah, right, 8155473 is the right commit. I have fixed the original post. Tough day today, sorry 😇

[kkos]

Yes, I think you're right.
I'm not sure what Coverity's output means now.
It's impossible to write a test case where this changes the results because the content of the optimization information changes only slightly.

[voxik]

I'm not sure what Coverity's output means now.

Neither am I, but I'll try to poke some Coverity experts.

[kdudka]

As discussed with @voxik, this is indeed a false positive of Coverity. The checker does not track non-trivial relations of integer variables. It sees that an array bound is checked by the condition of the outer for loop. Then it sees an increment of the index variable without subsequently checking the bound. The condition (i + len > OPT_EXACT_MAXLEN) is below the resolution of this checker because the condition evaluates a pair of integer variables.

Could we suppress the false positive with the following inline annotation in the source code?

--- a/src/regcomp.c
+++ b/src/regcomp.c
@@ -6260,18 +6260,19 @@ static void
 concat_opt_exact_str(OptStr* to, UChar* s, UChar* end, OnigEncoding enc)
 {
   int i, j, len;
   UChar *p;

   for (i = to->len, p = s; p < end && i < OPT_EXACT_MAXLEN; ) {
     len = enclen(enc, p);
     if (i + len > OPT_EXACT_MAXLEN) break;
     for (j = 0; j < len && p < end; j++)
+      /* coverity[overrun-local] */
       to->s[i++] = *p++;
   }

   to->len = i;

   if (p >= end)
     to->reach_end = 1;
 }