Skip to content

Assert sanity of name+len arguments to allocmy() #23583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025

Conversation

leonerd
Copy link
Contributor

@leonerd leonerd commented Aug 15, 2025

Recent discussons on github 1 found a bug when calling this function as

allocmy("", 0, 0)

This ought not be allowed. The length must be at least 2, because the function checks the first two characters of name.

  • This set of changes does not require a perldelta entry.

Recent discussons on github [1] found a bug when calling this function as

  allocmy("", 0, 0)

This ought not be allowed. The length must be at least 2, because the
function checks the first two characters of `name`.

[1]: Perl#23574 (comment)
@mauke
Copy link
Contributor

mauke commented Aug 15, 2025

Alternatively:

diff --cc op.c
index 6121c7c4f6,6121c7c4f6..5ce0c32181
--- op.c
+++ op.c
@@@ -771,9 -771,9 +771,11 @@@ Perl_allocmy(pTHX_ const char *const na
          croak("panic: allocmy illegal flag bits 0x%" UVxf,
                     (UV)flags);
  
--    is_idfirst = flags & SVf_UTF8
--        ? isIDFIRST_utf8_safe((U8*)name + 1, name + len)
--        : isIDFIRST_A(name[1]);
++    is_idfirst = len >= 2 && (
++        flags & SVf_UTF8
++            ? isIDFIRST_utf8_safe((U8*)name + 1, name + len)
++            : isIDFIRST_A(name[1])
++    );
  
      /* $_, @_, etc. */
      is_default = len == 2 && name[1] == '_';
@@@ -784,9 -784,9 +786,12 @@@
                PL_parser->in_my == KEY_sigvar ? "subroutine signature" :
                PL_parser->in_my == KEY_state  ? "\"state\""     : "\"my\"";
  
--        if (!(flags & SVf_UTF8 && UTF8_IS_START(name[1]))
--         && isASCII(name[1])
--         && (!isPRINT(name[1]) || memCHRs("\t\n\r\f", name[1]))) {
++        if (
++            len >= 2
++            && !(flags & SVf_UTF8 && UTF8_IS_START(name[1]))
++            && isASCII(name[1])
++            && (!isPRINT(name[1]) || memCHRs("\t\n\r\f", name[1]))
++        ) {
              /* diag_listed_as: Can't use global %s in %s */
              yyerror(form("Can't use global %c^%c%.*s in %s",
                                name[0], toCTRL(name[1]),

@leonerd
Copy link
Contributor Author

leonerd commented Aug 15, 2025

I thought about fixing it to allow it, but the only point of allocmy() is that it applies sanity checking of the name as you'd want from the parser, and applies some flags (again as you'd want from the parser). If you were passing an empty name you really wanted to just call pad_add_name_pvn directly.

@mauke
Copy link
Contributor

mauke commented Aug 15, 2025

Yeah, reasonable.

@leonerd leonerd merged commit 64e4a9d into Perl:blead Aug 15, 2025
34 checks passed
@leonerd leonerd deleted the assert-len-allocmy branch August 15, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants