Re: [PATCH, committed, part2] PR fortran/95090 - ICE: identifier overflow

[Jakub Jelinek via Gcc-patches]

On Sat, May 30, 2020 at 08:57:46PM +0200, Harald Anlauf wrote:
> > That is detecting it after the buffer overflow has happened already, that is
> > too late, after UB anything can happen.
> > +    {
> > +      const char *upper = gfc_dt_upper_string (derived->name);
> > +      size_t len = strnlen (upper, sizeof (dt_name));
> > +      gcc_assert (len < sizeof (dt_name));
> > +      memcpy (dt_name, upper, len);
> > +      dt_name[len] = '\0';
> > +    }
> > does detect it before overflowing it.
> 
> OK.  Here's what I committed:
> 
> 
>     PR fortran/95090 - ICE: identifier overflow
> 
>     Implement buffer overrun check for temporary that holds mangled names.
> 
>     2020-05-30  Harald Anlauf  <anl...@gmx.de>
> 
>     gcc/fortran/
>             PR fortran/95090
>             * class.c (get_unique_type_string): Use buffer overrun check.

Thanks.  Though, see Andreas' mail, I think he is right that there is really
no reason to have that dt_name local buffer at all, just changing
dt_name from an array to const char *dt_name; and changing the strcpy to
dt_name = "STAR";
and the former strncpy (dt_name, ...) to
dt_name = gfc_dt_upper_string (derived->name);
should do it.

There is a possible buffer overflow in the string with or without that
change but to fix that I think it would be desirable to pass not just the
string buffer to the function but also the length of the buffer and in the
function verify it will not overflow.  There is no reason to use sprintf
which is fairly expensive, and could be even simplified.

So, once dt_name is const char *, change that
  if (derived->attr.unlimited_polymorphic)
    sprintf (string, "_%s", dt_name);
  else if (derived->module)
    sprintf (string, "%s_%s", derived->module, dt_name);
  else if (derived->ns->proc_name)
    sprintf (string, "%s_%s", derived->ns->proc_name->name, dt_name);
  else
    sprintf (string, "_%s", dt_name);
to something like:
  const char *first = "";
  if (!derived->attr.unlimited_polymorphic)
    {
      if (derived->module)
        first = derived->module;
      else if (derived->ns->proc_name)
        first = derived->ns->proc_name->name;
    }
  size_t len1 = strlen (first), len2 = strlen (dt_name);
  if (len1 + 1 + len2 + 1 >= len) // len being the new passed argument - length 
of the buffer pointed by string
    gfc_internal_error (...);
  memcpy (string, first, len1);
  string[len1] = '_';
  memcpy (string + len1 + 1, dt_name, len2 + 1);

        Jakub