Python: `isType` treats any object in a module named `*typing` as a type, corrupting points-to for unrelated containers

Đang mở
#22,621 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Đánh giá

Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức phù hợp với người mới
68/100
Loại issue
Lỗi
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
python
Lĩnh vực
devtools, security

Hướng nghiên cứu

Start in python/ql/lib/semmle/python/objects/TObject.qll at isType and review how it feeds TSubscriptedType, then run NonCallableCalled.ql and HashedButNoHash.ql against the four-file reproduction. Compare results for pkg/typing.py and pkg/other.py, and verify that the false positives disappear without breaking handling of typing and typing_extensions modules.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

false-positive

Disclaimer

The analysis below was done by a coding agent. I tried to verify it up to my knowledge, but there could be inaccuracies. Nevertheless, the false positive is real and it has been affecting jsonargparse for a long time. Just decided that since I have access to a good coding agent I could analyze the issue and report it here.

Description of the false positive

py/call-to-non-callable and py/hash-unhashable-value fire on correct code purely because of the filename of the module the code lives in.

The cause is the second disjunct of isType in python/ql/lib/semmle/python/objects/TObject.qll#L215-L220:

predicate isType(ObjectInternal t) {
  t.isClass() = true
  or
  t.getOrigin().getEnclosingModule().getName().matches("%typing")
}

isType gates the construction of TSubscriptedType (TObject.qll#L200-L205), the model of a subscripted generic such as List[int]. Because the disjunct matches on module name rather than on anything about the object, every object defined in a module whose dotted name ends in typing is classified as a type. Subscripting an ordinary module-level dict in such a module therefore produces a SubscriptedTypeInternal, whose getClass() returns the generic's class (Classes.qll#L332):

override ObjectInternal getClass() { result = this.getGeneric().getClass() }

So my_registry[key] is inferred to be an instance of dict. Any function that returns such a lookup is then inferred to return a dict, and every downstream use of its result is flagged.

Note the alert message is quite misleading in this situation. On a class returned from a registry lookup, the user sees "Call to a non-callable of builtin-class dict" — the dict is the registry, not anything related to the value, and nothing in the message text says so. The origin in relatedLocations is the only clue, and it points at the registry lookup, often in a different file from the alert.

Code samples or links to source code

Self-contained reproduction. Four files; pkg/other.py is byte-identical to pkg/typing.py apart from the identifier names, and is there purely to isolate the filename as the trigger.

pkg/__init__.py (empty)

pkg/typing.py:

registry = {}


def make_type(name):
    if name in registry:
        return registry[name]

    class Created(str):
        pass

    registry[name] = Created
    return Created


InTyping = make_type("InTyping")

pkg/other.py: identical, with InTyping renamed to InOther.

main.py:

from typing import Optional

from pkg.other import InOther
from pkg.typing import InTyping

InTyping("a")  # py/call-to-non-callable  <- false positive
Optional[InTyping]  # py/hash-unhashable-value <- false positive

InOther("b")  # no alert
Optional[InOther]  # no alert

Running NonCallableCalled.ql and HashedButNoHash.ql from codeql/python-queries 1.6.8 (CLI 2.23.5) gives:

main.py:6  Non-callable called       Call to a non-callable of builtin-class dict.
                                     related location: pkg/typing.py:6:16:6:29
main.py:7  Unhashable object hashed  This instance of dict is unhashable.
                                     related location: pkg/typing.py:6:16:6:29

Both alerts are on uses of InTyping; the identical InOther uses produce nothing. The relatedLocation on both is pkg/typing.py:6, the registry[name] lookup.

This is not hypothetical. It is the cause of a long-standing batch of dismissed alerts in jsonargparse, which has a public jsonargparse.typing module:

Every py/call-to-non-callable and py/hash-unhashable-value alert in that repository traces, via relatedLocations, to that single line — including alerts in test files that merely call a path type such as Path_fr(...) or write Optional[Path_fr].

One further observation that may be useful when triaging severity: whether the corruption surfaces as an alert depends on unrelated downstream details. The same project has a second lookup of the same registry at typing.py#L248-L252 that produces the same bogus Dict[...] value, but it never escapes the function: the next statement reads registered_type.__name__, and since SubscriptedTypeInternal declares both attribute() { none() } and attributesUnknown() { none() }, the attribute lookup definitively fails and the following return becomes unreachable. Probing that return yields no points-to tuples at all. So the bad inference is present at both sites and happens to be masked at one of them.

Suggested fix

Restricting the disjunct to the actual typing modules removes the false positives:

predicate isType(ObjectInternal t) {
  t.isClass() = true
  or
  exists(string name | name = t.getOrigin().getEnclosingModule().getName() |
    name = "typing" or name = "typing_extensions" or name.matches("typing.%")
  )
}

With that change, on the reproduction above both alerts go to zero. On a database built from jsonargparse at the commit linked above, py/call-to-non-callable goes from 9 results to 0 and py/hash-unhashable-value from 11 to 0. Those before-counts are exactly what code scanning itself reported for that commit on main (9 and 11), so the narrowing clears the full set of false positives in that project.

Open question

I could not construct a case where the current disjunct actually helps, so I may be missing its intended purpose. In every database I built the stdlib typing module is not extracted at all — querying for modules matching %typing returned only the project's own jsonargparse.typing and jsonargparse_tests.test_typing. If the disjunct exists to model typing.List and friends via extracted stdlib source, a maintainer will know whether the narrowing above preserves that; if it only ever matches user modules that happen to be named *typing, it may be removable outright.

URL to the alert on GitHub code scanning (optional)

The affected alerts are dismissed as false positives in a public repository, e.g. https://github.com/mauvilsa/jsonargparse/security/code-scanning/617 — raised on path = Path_fr(file_r), where Path_fr is a class. (Code scanning alerts are only visible to users with write access even on public repositories, so that link may not be usable from outside the project; the reproduction above stands on its own.)

Environment

  • codeql-bundle-v2.23.5 — CodeQL CLI 2.23.5, codeql/python-all 4.1.0, codeql/python-queries 1.6.8
  • Also present on main as of 7a367c9b70e215e4e6373bbfd3164d4f44bcc11d
Ngôn ngữ chính
CodeQL
Star
10.1k
Fork
2.1k
Merge trung bình
2 ngày 11 giờ
Pull request đã merge (30 ngày)
129

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của github/codeql

Tất cả issue của github/codeql

Issue tương tự

Thêm issue về DevTools

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.