fix: only allow valid identifiers to be nonreentrant keys (#3605)

disallow invalid identifiers like `" "`, `"123abc"` from being keys for non-reentrant locks. this commit also refactors the `validate_identifiers` helper function to be in the `ast/` subdirectory, and slightly improves the VyperException constructor by allowing None (optional) annotations.
vyperlang · Sep 15, 2023 · 0b74028 · 0b74028
1 parent 344fd8f
commit 0b74028
Show file tree

Hide file tree

Showing 9 changed files with 147 additions and 126 deletions.
diff --git a/tests/parser/exceptions/test_structure_exception.py b/tests/parser/exceptions/test_structure_exception.py
@@ -56,9 +56,26 @@ def double_nonreentrant():
     """,
     """
 @external
-@nonreentrant("B")
-@nonreentrant("C")
-def double_nonreentrant():
+@nonreentrant(" ")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("123")
+def invalid_nonreentrant_key():
+    pass
+    """,
+    """
+@external
+@nonreentrant("!123abcd")
+def invalid_nonreentrant_key():
     pass
     """,
     """

diff --git a/tests/parser/features/decorators/test_nonreentrant.py b/tests/parser/features/decorators/test_nonreentrant.py
@@ -142,7 +142,7 @@ def set_callback(c: address):
 
 @external
 @payable
-@nonreentrant('default')
+@nonreentrant("lock")
 def protected_function(val: String[100], do_callback: bool) -> uint256:
     self.special_value = val
     _amount: uint256 = msg.value
@@ -166,7 +166,7 @@ def unprotected_function(val: String[100], do_callback: bool):
 
 @external
 @payable
-@nonreentrant('default')
+@nonreentrant("lock")
 def __default__():
     pass
     """

diff --git a/tests/parser/test_call_graph_stability.py b/tests/parser/test_call_graph_stability.py
@@ -6,8 +6,8 @@
 from hypothesis import given, settings
 
 import vyper.ast as vy_ast
+from vyper.ast.identifiers import RESERVED_KEYWORDS
 from vyper.compiler.phases import CompilerData
-from vyper.semantics.namespace import RESERVED_KEYWORDS
 
 
 def _valid_identifier(attr):

diff --git a/tests/parser/types/test_identifier_naming.py b/tests/parser/types/test_identifier_naming.py
@@ -1,10 +1,10 @@
 import pytest
 
 from vyper.ast.folding import BUILTIN_CONSTANTS
+from vyper.ast.identifiers import RESERVED_KEYWORDS
 from vyper.builtins.functions import BUILTIN_FUNCTIONS
 from vyper.codegen.expr import ENVIRONMENT_VARIABLES
 from vyper.exceptions import NamespaceCollision, StructureException, SyntaxException
-from vyper.semantics.namespace import RESERVED_KEYWORDS
 from vyper.semantics.types.primitives import AddressT
 
 BUILTIN_CONSTANTS = set(BUILTIN_CONSTANTS.keys())

diff --git a/vyper/ast/identifiers.py b/vyper/ast/identifiers.py
@@ -0,0 +1,111 @@
+import re
+
+from vyper.exceptions import StructureException
+
+
+def validate_identifier(attr, ast_node=None):
+    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
+        raise StructureException(f"'{attr}' contains invalid character(s)", ast_node)
+    if attr.lower() in RESERVED_KEYWORDS:
+        raise StructureException(f"'{attr}' is a reserved keyword", ast_node)
+
+
+# https://docs.python.org/3/reference/lexical_analysis.html#keywords
+# note we don't technically need to block all python reserved keywords,
+# but do it for hygiene
+_PYTHON_RESERVED_KEYWORDS = {
+    "False",
+    "None",
+    "True",
+    "and",
+    "as",
+    "assert",
+    "async",
+    "await",
+    "break",
+    "class",
+    "continue",
+    "def",
+    "del",
+    "elif",
+    "else",
+    "except",
+    "finally",
+    "for",
+    "from",
+    "global",
+    "if",
+    "import",
+    "in",
+    "is",
+    "lambda",
+    "nonlocal",
+    "not",
+    "or",
+    "pass",
+    "raise",
+    "return",
+    "try",
+    "while",
+    "with",
+    "yield",
+}
+_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}
+
+# Cannot be used for variable or member naming
+RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
+    # decorators
+    "public",
+    "external",
+    "nonpayable",
+    "constant",
+    "immutable",
+    "transient",
+    "internal",
+    "payable",
+    "nonreentrant",
+    # "class" keywords
+    "interface",
+    "struct",
+    "event",
+    "enum",
+    # EVM operations
+    "unreachable",
+    # special functions (no name mangling)
+    "init",
+    "_init_",
+    "___init___",
+    "____init____",
+    "default",
+    "_default_",
+    "___default___",
+    "____default____",
+    # more control flow and special operations
+    "range",
+    # more special operations
+    "indexed",
+    # denominations
+    "ether",
+    "wei",
+    "finney",
+    "szabo",
+    "shannon",
+    "lovelace",
+    "ada",
+    "babbage",
+    "gwei",
+    "kwei",
+    "mwei",
+    "twei",
+    "pwei",
+    # sentinal constant values
+    # TODO remove when these are removed from the language
+    "zero_address",
+    "empty_bytes32",
+    "max_int128",
+    "min_int128",
+    "max_decimal",
+    "min_decimal",
+    "max_uint256",
+    "zero_wei",
+}
diff --git a/vyper/exceptions.py b/vyper/exceptions.py
@@ -54,7 +54,9 @@ def __init__(self, message="Error Message not found.", *items):
             # support older exceptions that don't annotate - remove this in the future!
             self.lineno, self.col_offset = items[0][:2]
         else:
-            self.annotations = items
+            # strip out None sources so that None can be passed as a valid
+            # annotation (in case it is only available optionally)
+            self.annotations = [k for k in items if k is not None]
 
     def with_annotation(self, *annotations):
         """

diff --git a/vyper/semantics/namespace.py b/vyper/semantics/namespace.py
@@ -1,12 +1,7 @@
 import contextlib
-import re
-
-from vyper.exceptions import (
-    CompilerPanic,
-    NamespaceCollision,
-    StructureException,
-    UndeclaredDefinition,
-)
+
+from vyper.ast.identifiers import validate_identifier
+from vyper.exceptions import CompilerPanic, NamespaceCollision, UndeclaredDefinition
 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
 
 
@@ -121,111 +116,3 @@ def override_global_namespace(ns):
     finally:
         # unclobber
         _namespace = tmp
-
-
-def validate_identifier(attr):
-    if not re.match("^[_a-zA-Z][a-zA-Z0-9_]*$", attr):
-        raise StructureException(f"'{attr}' contains invalid character(s)")
-    if attr.lower() in RESERVED_KEYWORDS:
-        raise StructureException(f"'{attr}' is a reserved keyword")
-
-
-# https://docs.python.org/3/reference/lexical_analysis.html#keywords
-# note we don't technically need to block all python reserved keywords,
-# but do it for hygiene
-_PYTHON_RESERVED_KEYWORDS = {
-    "False",
-    "None",
-    "True",
-    "and",
-    "as",
-    "assert",
-    "async",
-    "await",
-    "break",
-    "class",
-    "continue",
-    "def",
-    "del",
-    "elif",
-    "else",
-    "except",
-    "finally",
-    "for",
-    "from",
-    "global",
-    "if",
-    "import",
-    "in",
-    "is",
-    "lambda",
-    "nonlocal",
-    "not",
-    "or",
-    "pass",
-    "raise",
-    "return",
-    "try",
-    "while",
-    "with",
-    "yield",
-}
-_PYTHON_RESERVED_KEYWORDS = {s.lower() for s in _PYTHON_RESERVED_KEYWORDS}
-
-# Cannot be used for variable or member naming
-RESERVED_KEYWORDS = _PYTHON_RESERVED_KEYWORDS | {
-    # decorators
-    "public",
-    "external",
-    "nonpayable",
-    "constant",
-    "immutable",
-    "transient",
-    "internal",
-    "payable",
-    "nonreentrant",
-    # "class" keywords
-    "interface",
-    "struct",
-    "event",
-    "enum",
-    # EVM operations
-    "unreachable",
-    # special functions (no name mangling)
-    "init",
-    "_init_",
-    "___init___",
-    "____init____",
-    "default",
-    "_default_",
-    "___default___",
-    "____default____",
-    # more control flow and special operations
-    "range",
-    # more special operations
-    "indexed",
-    # denominations
-    "ether",
-    "wei",
-    "finney",
-    "szabo",
-    "shannon",
-    "lovelace",
-    "ada",
-    "babbage",
-    "gwei",
-    "kwei",
-    "mwei",
-    "twei",
-    "pwei",
-    # sentinal constant values
-    # TODO remove when these are removed from the language
-    "zero_address",
-    "empty_bytes32",
-    "max_int128",
-    "min_int128",
-    "max_decimal",
-    "min_decimal",
-    "max_uint256",
-    "zero_wei",
-}
diff --git a/vyper/semantics/types/base.py b/vyper/semantics/types/base.py
@@ -3,6 +3,7 @@
 
 from vyper import ast as vy_ast
 from vyper.abi_types import ABIType
+from vyper.ast.identifiers import validate_identifier
 from vyper.exceptions import (
     CompilerPanic,
     InvalidLiteral,
@@ -12,7 +13,6 @@
     UnknownAttribute,
 )
 from vyper.semantics.analysis.levenshtein_utils import get_levenshtein_error_suggestions
-from vyper.semantics.namespace import validate_identifier
 
 
 # Some fake type with an overridden `compare_type` which accepts any RHS

diff --git a/vyper/semantics/types/function.py b/vyper/semantics/types/function.py
@@ -5,6 +5,7 @@
 from typing import Any, Dict, List, Optional, Tuple
 
 from vyper import ast as vy_ast
+from vyper.ast.identifiers import validate_identifier
 from vyper.ast.validation import validate_call_args
 from vyper.exceptions import (
     ArgumentException,
@@ -220,7 +221,10 @@ def from_FunctionDef(
                         msg = "Nonreentrant decorator disallowed on `__init__`"
                         raise FunctionDeclarationException(msg, decorator)
 
-                    kwargs["nonreentrant"] = decorator.args[0].value
+                    nonreentrant_key = decorator.args[0].value
+                    validate_identifier(nonreentrant_key, decorator.args[0])
+
+                    kwargs["nonreentrant"] = nonreentrant_key
 
                 elif isinstance(decorator, vy_ast.Name):
                     if FunctionVisibility.is_valid_value(decorator.id):