[Bug c/106332] New: Possible out of bound buffer access in opts-common.c

[Bug c/106332] New: Possible out of bound buffer access in opts-common.c

[liftdat at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

            Bug ID: 106332
           Summary: Possible out of bound buffer access in opts-common.c
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: liftdat at protonmail dot com
  Target Milestone: ---

In the file gcc/opts-common.cc, the function candidates_list_and_hint has the
following code (link:
https://github.com/gcc-mirror/gcc/blob/9715f10c0651c9549b479b69d67be50ac4bd98a6/gcc/opts-common.cc#L1342):

const char *
candidates_list_and_hint (const char *arg, char *&str,
                          const auto_vec <const char *> &candidates)
{
  size_t len = 0;
  int i;
  const char *candidate;
  char *p;

  FOR_EACH_VEC_ELT (candidates, i, candidate)
    len += strlen (candidate) + 1;

  str = p = XNEWVEC (char, len);
  FOR_EACH_VEC_ELT (candidates, i, candidate)
    {
      len = strlen (candidate);
      memcpy (p, candidate, len);
      p[len] = ' ';
      p += len + 1;
    }
  p[-1] = '\0';
  return find_closest_string (arg, &candidates);
}

When candidates is an empty vector, the buffer access p[-1] is out of bound.

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The one inside check_offload_target_name (in gcc.cc) will always have at least
one entry in candidates.

The one inside cmdline_handle_error (in opts-common.cc) could in theory be
called with no elements for candidates but if there was none, the option itself
would have been disabled before hand.

The two in config/aarch64.cc:
aarch64_print_hint_for_core_or_arch: there will always be at least one
candidate since there is always more than one core/arch defined.
aarch64_print_hint_for_extensions: there is always at least one candidate as
extensions will always be more than one



The few in config/i386.cc:
ix86_parse_stringop_strategy_string: at least one stringop_strategy
ix86_option_override_internal (both of them): at least one processor defined



So the question is how did you find this?

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I suppose we could just put a gcc_assert in there to make that assumption
explicit and give an ICE in checked builds.

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[liftdat at protonmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

--- Comment #3 from liftdat at protonmail dot com ---
(In reply to Andrew Pinski from comment #1)
> The one inside check_offload_target_name (in gcc.cc) will always have at
> least one entry in candidates.
> 
> The one inside cmdline_handle_error (in opts-common.cc) could in theory be
> called with no elements for candidates but if there was none, the option
> itself would have been disabled before hand.
> 
> The two in config/aarch64.cc:
> aarch64_print_hint_for_core_or_arch: there will always be at least one
> candidate since there is always more than one core/arch defined.
> aarch64_print_hint_for_extensions: there is always at least one candidate as
> extensions will always be more than one
> 
> 
> 
> The few in config/i386.cc:
> ix86_parse_stringop_strategy_string: at least one stringop_strategy
> ix86_option_override_internal (both of them): at least one processor defined
> 
> 
> 
> So the question is how did you find this?

This is found by a static analysis tool. I think putting an assertion can help
to clarify the requirements for any future clients of this function.

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #4 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to liftdat from comment #3)
> (In reply to Andrew Pinski from comment #1)
> > The one inside check_offload_target_name (in gcc.cc) will always have at
> > least one entry in candidates.
> > 
> > The one inside cmdline_handle_error (in opts-common.cc) could in theory be
> > called with no elements for candidates but if there was none, the option
> > itself would have been disabled before hand.
> > 
> > The two in config/aarch64.cc:
> > aarch64_print_hint_for_core_or_arch: there will always be at least one
> > candidate since there is always more than one core/arch defined.
> > aarch64_print_hint_for_extensions: there is always at least one candidate as
> > extensions will always be more than one
> > 
> > 
> > 
> > The few in config/i386.cc:
> > ix86_parse_stringop_strategy_string: at least one stringop_strategy
> > ix86_option_override_internal (both of them): at least one processor defined
> > 
> > 
> > 
> > So the question is how did you find this?
> 
> This is found by a static analysis tool.

Could you be a bit more specific about which static analysis tool?

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:cc078cf85295ec5d0a63a16afbd045efac0d455e

commit r13-1855-gcc078cf85295ec5d0a63a16afbd045efac0d455e
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Jul 27 12:04:50 2022 +0200

    opts: Add an assertion to help static analyzers [PR106332]

    This function would have UB if called with empty candidates vector
    (accessing p[-1] where p is malloc (0) result).
    As analyzed in the PR, we never call it with empty vector, so this just
    adds an assertion to make it clear.

    2022-07-27  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/106332
            * opts-common.cc (candidates_list_and_hint): Add gcc_assert
            that candidates is not an empty vector.

[Bug middle-end/106332] Possible out of bound buffer access in opts-common.c

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106332

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Assert added.  Closing as INVALID because it wasn't really a bug.