LGTM.com - false positive: Python metaclass

[gpotter2]

Description of the false positive
Classes may have metaclasses in Python. If the metaclass has a __call__ function, then calling the class should require the same number of arguments than the __call__ function of the metaclass has.
See https://docs.python.org/3/reference/datamodel.html#object.call

URL to the alert on the project page on LGTM.com: https://lgtm.com/projects/g/secdev/scapy/snapshot/4d06eb2812a98a1001478bf06e033fbbb2afbef7/files/scapy/sendrecv.py#L844

See:

Call to PcapReader.init with too few arguments; should be no fewer than 3.

If you need more help understanding the issue ping me and I'll make a snippet. This code isn't the easiest to catch on

[embray]

Came here to report the same. This affects:

• py/call/wrong-named-class-argument
• py/call/wrong-number-class-arguments

Here is some simple example code that demonstrates the issue (Python 3.x):

>>> class Meta(type):
...     def __call__(cls, *args, **kwargs):
...         print('creating an instance of', cls, 'without calling its __init__')
...         print('args:', args)
...         print('kwargs:', kwargs)
...         return cls.__new__(cls)
...
>>> class A(metaclass=Meta):
...     def __init__(self):
...         print('should never be called')
...
>>> a = A(1, 2, 3, a=4, b=5, c=6)
creating an instance of <class '__main__.A'> without calling its __init__
args: (1, 2, 3)
kwargs: {'c': 6, 'a': 4, 'b': 5}
>>> a
<__main__.A object at 0x7f54957b9668>

This code will produce a false positive, because even though the class A has an __init__ method, it is never called (or could be called, but with different arguments) when the class A is called.

This may seem obscure but the SageMath project uses custom class calls extensively in its dynamic class system. A large number of its reported errors are false positives due to this (we are discussing disabling them globally which I think is a perfectly acceptable work-around for now. I've never poked around at how CodeQL works so I have no idea how hard it is to check this case, since with metaclasses you can have more-or-less arbitrary behavior.

[johnandersen777]

We have the same issue over in https://github.com/intel/dffml

class BaseConfigurableMetaClass(type, abc.ABC):
    def __new__(cls, name, bases, props, module=None):
        # Create the class
        cls = super(BaseConfigurableMetaClass, cls).__new__(
            cls, name, bases, props
        )
        # Wrap __init__
        setattr(cls, "__init__", cls.wrap(cls.__init__))
        return cls


    @classmethod
    def wrap(cls, func):
        """
        If a subclass of BaseConfigurable is passed keyword arguments, convert
        them into the instance of the CONFIG class.
        """


        @functools.wraps(func)
        def wrapper(self, config: Optional[BaseConfig] = None, **kwargs):
            if config is not None and len(kwargs):
                raise ConfigAndKWArgsMutuallyExclusive
            elif config is None and hasattr(self, "CONFIG") and kwargs:
                try:
                    config = self.CONFIG(**kwargs)
                except TypeError as error:
                    error.args = (
                        error.args[0].replace(
                            "__init__", f"{self.CONFIG.__qualname__}"
                        ),
                    )
                    raise
            if config is None:
                raise TypeError(
                    "__init__() missing 1 required positional argument: 'config'"
                )
            return func(self, config)


        return wrapper




class BaseConfigurable(metaclass=BaseConfigurableMetaClass):
    """
    Class which produces a config for itself by providing Args to a CMD (from
    dffml.util.cli.base) and then using a CMD after it contains parsed args to
    instantiate a config (deriving from BaseConfig) which will be used as the
    only parameter to the __init__ of a BaseDataFlowFacilitatorObject.
    """


    def __init__(self, config: Type[BaseConfig]) -> None:
        """
        BaseConfigurable takes only one argument to __init__,
        its config, which should inherit from BaseConfig. It shall be a object
        containing any information needed to configure the class and it's child
        context's.
        """
        self.config = config
        str_config = str(self.config)
        self.logger.debug(
            str_config if len(str_config) < 512 else (str_config[:512] + "...")
        )

[RasmusWL]

Hi folks. This must have fallen off our radar. @BekaValentine I know you have been looking at these queries just now, could you have a look whether this would be easy to fix?

[embray]

Part of the problem is that it's probably not easy to fix, since metaclasses can do almost anything here that would make static analysis close to impossible. Maybe at best, if a class has a custom metaclass, change this to a warning.

[gpotter2]

Correct me if I'm wrong but it should be possible to at least fix the expected number of arguments?

[embray]

Not necessarily. I mean, I could write a metaclass that randomizes the number of expected arguments on every call. At best you can say that it's unknowable.

[RasmusWL]

I could write a metaclass that randomizes the number of expected arguments on every call. At best you can say that it's unknowable.

@embray that's essentially what our query is doing. If we can statically determine the minimum number of arguments required, and can see you're not providing enough, we will give you an alert. So if you have the function definitions

def foo(*args):
    ...

def bar(x, *args):
    ...

we can give an alert if you call bar(), since you must provide at least one argument, but we won't give an alert for any call to foo since we know it's valid to call it without arguments.

Currently we don't take metaclasses into account, so once we do that, we hopefully shouldn't see these False Positives anymore.

[embray]

That doesn't necessarily help because you can't even know what methods are involved in initializing instances of a given class. Here's an example of what I'm talking about:

>>> import random
>>> class Meta(type):
...     @staticmethod
...     def __meta_init__(self, a, b, c):
...         """Sometimes this is used to initialize instances of classes
...         with this metaclass instead of their own __init__
...         """
...         print(f'__meta_init__({self}, {a}, {b}, {c})')
...
...     def __call__(cls, *args):
...         self = cls.__new__(cls, *args)
...         if random.random() < 0.5:
...             self.__init__(*args)
...         else:
...             cls.__meta_init__(self, *args)
...         return self
...     
>>> class A(metaclass=Meta):
...     def __init__(self, a, b):
...         print(f'A.__init__({a}, {b})')
...         
>>> A(1, 2)
Traceback (most recent call last):
  File "<ipython-input-8-dd284f58b13e>", line 1, in <module>
    A(1, 2)
  File "<ipython-input-6-9c14207cf8e4>", line 13, in __call__
    cls.__meta_init__(self, *args)
TypeError: __meta_init__() missing 1 required positional argument: 'c'

>>> A(1, 2)
Traceback (most recent call last):
  File "<ipython-input-9-dd284f58b13e>", line 1, in <module>
    A(1, 2)
  File "<ipython-input-6-9c14207cf8e4>", line 13, in __call__
    cls.__meta_init__(self, *args)
TypeError: __meta_init__() missing 1 required positional argument: 'c'

>>> A(1, 2)
A.__init__(1, 2)
<__main__.A object at 0x7f860ad7b7b8>
>>> A(1, 2, 3)
Traceback (most recent call last):
  File "<ipython-input-11-f4db6b51352e>", line 1, in <module>
    A(1, 2, 3)
  File "<ipython-input-6-9c14207cf8e4>", line 11, in __call__
    self.__init__(*args)
TypeError: __init__() takes 3 positional arguments but 4 were given

>>> A(1, 2, 3)
__meta_init__(<__main__.A object at 0x7f860ad66b38>, 1, 2, 3)
<__main__.A object at 0x7f860ad66b38>

Obviously this case is completely pathological, but one can think of other cases, such with classes that are created at runtime (Sage for example is full of these).

[BekaValentine]

I've tried to recreate this issue but I've been unable to. I have a branch here:

https://github.com/BekaValentine/ql/tree/python-false-positive-metaclass-call

Just over a week ago, a PR merged that updated the API that's used in these queries, which may be the reason, but I also tried to recreate this on the older API, and was still unable to get it working: https://github.com/BekaValentine/ql/tree/python-false-positive-metaclass-call-objectapi

@RasmusWL have you had any luck recreating this?

[johnandersen777]

My issue comes from the fact that the metaclass instantiates an object with a default arg1 if no parameters are passed. Which doesn't really fit with the "at least this number" check