bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

An confusing Coverity issue in regcomp.c


From: Darren Kenny
Subject: An confusing Coverity issue in regcomp.c
Date: Thu, 24 Mar 2022 17:32:36 +0000

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]