Hi,
As mentioned in another e-mail, I'm working on GRUB which makes use of a
built-in version of the gnulib code to enable it to function.
While running Coverity on this code, I'm seeing the following error
where Coverity believes that the return value from re_compile_internal()
may contain a value between -1 and 31.
I would like to determine the correct approach or fix to this issue, but
it isn't clear-cut and I would like your advice on it.
What I cannot figure out is whether it is actually somehow possible or
not - it doesn't appear to be given that the return value is a
reg_errcode_t enumeration - which for most cases has integer values in
the range 0 to 17. (more below on that).
203 const char *
204 re_compile_pattern (const char *pattern, size_t length,
205 struct re_pattern_buffer *bufp)
206 {
207 reg_errcode_t ret;
208
209 /* And GNU code determines whether or not to get register information
210 by passing null for the REGS argument to re_match, etc., not by
211 setting no_sub, unless RE_NO_SUB is set. */
1. Condition !!(rpl_re_syntax_options & (33554432UL /*
(((((((((((((((((((((((((unsigned long)1 << 1) << 1) << 1) << 1) << 1) << 1) <<
1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) << 1) <<
1) << 1) << 1) << 1) << 1) << 1 */)), taking true branch.
212 bufp->no_sub = !!(re_syntax_options & RE_NO_SUB);
213
214 /* Match anchors at newline. */
215 bufp->newline_anchor = 1;
216
2. assignment: Assigning: ret = re_compile_internal(bufp, pattern,
length, rpl_re_syntax_options). The value of ret is now between -1 and 31
(inclusive).
217 ret = re_compile_internal (bufp, pattern, length, re_syntax_options);
218
3. Condition !ret, taking false branch.
219 if (!ret)
220 return NULL;
CID 375038 (#1 of 1): Out-of-bounds read (OVERRUN)
4. overrun-local: Overrunning array __re_error_msgid_idx of 17
8-byte elements at element index 31 (byte offset 255) using index (int)ret
(which evaluates to 31).
221 return gettext (__re_error_msgid + __re_error_msgid_idx[(int) ret]);
222 }
I've considered putting a DEBUG_ASSERT() to test the range, but that
seems like a cop-out rather than addressing the issue.
When I mentioned that reg_errcode_t had the values from 0 to 16, there
is one circumstance where it may also have a -1 value, as can be seen in
its definition:
346 typedef enum
347 {
348 _REG_ENOSYS = -1, /* This will never happen for this
implementation. */
349 _REG_NOERROR = 0, /* Success. */
350 _REG_NOMATCH, /* Didn't find a match (for regexec). */
351
352 /* POSIX regcomp return error codes. (In the order listed in the
353 standard.) */
354 _REG_BADPAT, /* Invalid pattern. */
355 _REG_ECOLLATE, /* Invalid collating element. */
356 _REG_ECTYPE, /* Invalid character class name. */
357 _REG_EESCAPE, /* Trailing backslash. */
358 _REG_ESUBREG, /* Invalid back reference. */
359 _REG_EBRACK, /* Unmatched left bracket. */
360 _REG_EPAREN, /* Parenthesis imbalance. */
361 _REG_EBRACE, /* Unmatched \{. */
362 _REG_BADBR, /* Invalid contents of \{\}. */
363 _REG_ERANGE, /* Invalid range end. */
364 _REG_ESPACE, /* Ran out of memory. */
365 _REG_BADRPT, /* No preceding re for repetition op. */
366
367 /* Error codes we've added. */
368 _REG_EEND, /* Premature end. */
369 _REG_ESIZE, /* Too large (e.g., repeat count too large). */
370 _REG_ERPAREN /* Unmatched ) or \); not returned from
regcomp. */
371 } reg_errcode_t;
372
373 #if defined _XOPEN_SOURCE || defined __USE_XOPEN2K
374 # define REG_ENOSYS _REG_ENOSYS
375 #endif
While the comment above suggests that -1 will never happen for this
implementation, and REG_ENOSYS is only defined when _XOPEN_SOURCE or
__USE_XOPEN2K are defined (which I believe GRUB doesn't do), it seems
remiss to assume that the enumeration value can be used as an array
index when it could possibly (although unlikely to) have a value of -1.
Should the code ensure that this could never happen by only ever
defining the -1 value with those above defines in place, and similarly,
with the use as an array index have some appropriate code, also
wrapped in those defines, to test if it happens to have a -1 value?
Thanks,
Darren
