[PATCH 15/19] RISC-V: avoid buffer underrun in subset parsing

[Jiawei]

> In both instances 'p' can be the first character. Inspecting the character
> immediately ahead of it is then UB.
>
> To increase similarity between the two checks (each), also move the
> increment of q past the check for the (bogus) "<number>p" ending.
>
> Correct indentation in nearby code, where it being wrong is actively
> misleading.
> ---
> Why is it that P is recognized as an extension when there's no other
> support there for it?
>
LGTM.


Regarding the question about P, the P extension is still in draft state:

https://github.com/riscv/riscv-p-spec

We already have some downstream experimental support for it. Once the P
specification is frozen, we plan to prepare and submit the corresponding
binutils support upstream.

So I think the P-related cleanup can be handled separately from this UB fix.

>
> --- a/bfd/elfxx-riscv.c
> +++ b/bfd/elfxx-riscv.c
> @@ -2113,25 +2113,27 @@ riscv_parse_extensions (riscv_parse_subs
>          find_any_version = true;
>            else if (find_any_version
>                 && !find_minor_version
> +               && q > subset
>                 && *q == 'p'
>                 && ISDIGIT (*(q - 1)))
> -          find_minor_version = true;
> +        find_minor_version = true;
>            else
>          break;
>          }
> -      q++;
>
>        /* Check if the end of extension is 'p' or not.  If yes, then
>           the second letter from the end cannot be number.  */
> -      if (*(q - 1) == 'p' && ISDIGIT (*(q - 2)))
> +      if (q > subset && *q == 'p' && ISDIGIT (*(q - 1)))
>          {
> -          *q = '\0';
> +          q[1] = '\0';
>            rps->error_handler
>          (_("%s: invalid prefixed ISA extension `%s' ends with 
> <number>p"),
>           arch, subset);
>            free (subset);
>            return NULL;
>          }
> +
> +      q++;
>      }
>
>        int major_version = RISCV_UNKNOWN_VERSION;
> @@ -2672,20 +2674,19 @@ riscv_update_subset1 (riscv_parse_subset
>          find_any_version = true;
>        else if (find_any_version
>             && !find_minor_version
> +           && q > subset
>             && *q == 'p'
>             && ISDIGIT (*(q - 1)))
>          find_minor_version = true;
>        else
>          break;
>      }
> -      if (len > 0)
> -    q++;
>
>        /* Check if the end of extension is 'p' or not.  If yes, then
>       the second letter from the end cannot be number.  */
> -      if (len > 1 && *(q - 1) == 'p' && ISDIGIT (*(q - 2)))
> +      if (q > subset && *q == 'p' && ISDIGIT (*(q - 1)))
>      {
> -      *q = '\0';
> +      q[1] = '\0';
>        rps->error_handler
>          (_("%sinvalid ISA extension ends with <number>p in %s `%s'"),
>             errmsg_internal, errmsg_caller, implicit_exts);
> @@ -2693,6 +2694,9 @@ riscv_update_subset1 (riscv_parse_subset
>        return false;
>      }
>
> +      if (len > 0)
> +    q++;
> +
>        end_of_version =
>      riscv_parsing_subset_version (q, &major_version, &minor_version);
>        *q = '\0';
>
Reviewed-by: Jiawei <jiawei@iscas.ac.cn>