[JIT] Disallow plain List type annotation without arg by SplitInfinity · Pull Request #38130 · pytorch/pytorch

SplitInfinity · 2020-05-08T17:47:48Z

Summary
This commit detects and prohibits the case in which typing.List is
used as an annotation without a type argument (i.e. typing.List[T]).
At present, typing.List is always assumed to have one argument, and
when it is used without one, typing.List.__args__[0] is nonempty and
set to some typing.TypeVar instance, which has no JIT type equivalent.
Consequently, trying to convert typing.List to a JIT type results in
a c10::ListType with nullptr for its element type, which can cause
a segmentation fault.

This is fixed by returning a ListType from
jit.annotations.try_ann_to_type only if the element type is converted
successfully to a JIT type and returning None otherwise.

Test Plan
I ran the code from the issue (#37530) that reported this problem and also ran some unit tests.

Before

$ python3 segfault.py 
Segmentation fault (core dumped)

After

$ python3 segfault.py 
Traceback (most recent call last):
...
RuntimeError: 
Unknown type name 'List':
  File "segfault.py", line 9
    @classmethod
    def cat(cls, box_lists: List):
                            ~~~~ <--- HERE
        return cls(torch.cat([x for x in box_lists]))
'Boxes.cat' is being compiled since it was called from 'Boxes'
  File "segfault.py", line 13
def f(t: torch.Tensor):
    b = Boxes(t)
        ~~~~~ <--- HERE
    c = Boxes(torch.tensor([3, 4]))
    return Boxes.cat([b, c])
'Boxes' is being compiled since it was called from 'f'
  File "segfault.py", line 13
def f(t: torch.Tensor):
    b = Boxes(t)
    ~~~~~~~~~~~ <--- HERE
    c = Boxes(torch.tensor([3, 4]))
    return Boxes.cat([b, c])

Fixes
This pull request fixes #37530.

SplitInfinity · 2020-05-08T17:51:35Z

Note that an annotation of Dict with no key or value annotation can also cause a segmentation fault (for the same reason).

suo

Nice! Can we add a test that asserts that a failure is properly thrown when we hit this condition? In particular, I want to see if the string frontend is affected by this failure as well.

Summary: This commit detects and prohibits the case in which `typing.List` is used as an annotation without a type argument (i.e. `typing.List[T]`). At present, `typing.List` is always assumed to have one argument, and when it is used without one, `typing.List.__args__[0]` is nonempty and set to some `typing.TypeVar` instance, which has no JIT type equivalent. Consequently, trying to convert `typing.List` to a JIT type results in a `c10::ListType` with `nullptr` for its element type, which can cause a segmentation fault. This is fixed by returning a `ListType` from `jit.annotations.try_ann_to_type` only if the element type is converted successfully to a JIT type and returning `None` otherwise. Test Plan: I ran the code from the issue that reported this segmentation fault, and also added some unit tests.

SplitInfinity · 2020-05-08T20:30:58Z

I think both are affected because the problem is in the PythonResolver that is used either way:

  TypePtr resolveType(const std::string& name, const SourceRange& loc)
      override {
<some stuff>

    auto annotation_type = py::module::import("torch.jit.annotations")
                               .attr("try_ann_to_type")(obj, loc);
    if (!annotation_type.is_none()) {
      return py::cast<TypePtr>(annotation_type);
    }
    return resolveTypeFromObject(obj, loc);
  }

suo

lgtm! minor comment inline

suo · 2020-05-08T20:59:14Z

+            a: List = x.tolist()
+            return a
+
+        with self.assertRaisesRegex(RuntimeError, r"Unknown type name"):


self.checkScriptRaisesRegex is a useful utility for doing this; it will check eager/source FE/ast FE and compare results.

No exception is raised in eager though, and there is no way to opt out of asserting that eager raises the exception too.

SplitInfinity · 2020-05-08T21:22:45Z

Also, should we wait until someone tries to use an Dict annotation with no key/value types, or fix that now as well?

facebook-github-bot

@SplitInfinity has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@SplitInfinity has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-05-12T00:17:19Z

@SplitInfinity merged this pull request in 1456515.

**Summary** This commit extends the work done in #38130 and disallows plain Python3-style `List` type annotations. **Test Plan** This commit extends `TestList.test_no_element_type_annotation` to the Python3-style type annotation. [ghstack-poisoned]

Summary: Pull Request resolved: #44584 **Summary** This commit extends the work done in #38130 and disallows plain Python3-style `List` type annotations. **Test Plan** This commit extends `TestList.test_no_element_type_annotation` to the Python3-style type annotation. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23721514 Pulled By: SplitInfinity fbshipit-source-id: 48957868286f44ab6d5bf5e1bf97f0a4ebf955df

Summary: **Summary** This commit detects and prohibits the case in which `typing.List` is used as an annotation without a type argument (i.e. `typing.List[T]`). At present, `typing.List` is always assumed to have one argument, and when it is used without one, `typing.List.__args__[0]` is nonempty and set to some `typing.TypeVar` instance, which has no JIT type equivalent. Consequently, trying to convert `typing.List` to a JIT type results in a `c10::ListType` with `nullptr` for its element type, which can cause a segmentation fault. This is fixed by returning a `ListType` from `jit.annotations.try_ann_to_type` only if the element type is converted successfully to a JIT type and returning `None` otherwise. **Test Plan** I ran the code from the issue (pytorch#37530) that reported this problem and also ran some unit tests. *Before* ``` $ python3 segfault.py Segmentation fault (core dumped) ``` *After* ``` $ python3 segfault.py Traceback (most recent call last): ... RuntimeError: Unknown type name 'List': File "segfault.py", line 9 classmethod def cat(cls, box_lists: List): ~~~~ <--- HERE return cls(torch.cat([x for x in box_lists])) 'Boxes.cat' is being compiled since it was called from 'Boxes' File "segfault.py", line 13 def f(t: torch.Tensor): b = Boxes(t) ~~~~~ <--- HERE c = Boxes(torch.tensor([3, 4])) return Boxes.cat([b, c]) 'Boxes' is being compiled since it was called from 'f' File "segfault.py", line 13 def f(t: torch.Tensor): b = Boxes(t) ~~~~~~~~~~~ <--- HERE c = Boxes(torch.tensor([3, 4])) return Boxes.cat([b, c]) ``` **Fixes** This pull request fixes pytorch#37530. Pull Request resolved: pytorch#38130 Differential Revision: D21485284 Pulled By: SplitInfinity fbshipit-source-id: 9b51ef6340485a24c8b7cfb85832d4668b8ac51a

Summary: Pull Request resolved: pytorch#44584 **Summary** This commit extends the work done in pytorch#38130 and disallows plain Python3-style `List` type annotations. **Test Plan** This commit extends `TestList.test_no_element_type_annotation` to the Python3-style type annotation. Test Plan: Imported from OSS Reviewed By: gmagogsfm Differential Revision: D23721514 Pulled By: SplitInfinity fbshipit-source-id: 48957868286f44ab6d5bf5e1bf97f0a4ebf955df

SplitInfinity requested a review from apaszke as a code owner May 8, 2020 17:47

SplitInfinity requested review from eellison, suo and zdevito May 8, 2020 17:48

facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 8, 2020

suo reviewed May 8, 2020

View reviewed changes

SplitInfinity force-pushed the segfault branch from b1ac3ff to 7cb1373 Compare May 8, 2020 20:10

SplitInfinity requested a review from suo May 8, 2020 20:33

suo approved these changes May 8, 2020

View reviewed changes

facebook-github-bot reviewed May 8, 2020

View reviewed changes

facebook-github-bot reviewed May 9, 2020

View reviewed changes

facebook-github-bot closed this in 1456515 May 11, 2020

facebook-github-bot added the merged label May 12, 2020

SplitInfinity mentioned this pull request Sep 2, 2020

Segmentation fault with TorchScript while scripting a function with an argument Optional[List] #42859

Closed

SplitInfinity mentioned this pull request Sep 12, 2020

[JIT] Disallow plain List type annotation without arg #44584

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] Disallow plain List type annotation without arg#38130

[JIT] Disallow plain List type annotation without arg#38130
SplitInfinity wants to merge 1 commit intopytorch:masterfrom
SplitInfinity:segfault

SplitInfinity commented May 8, 2020 •

edited

Loading

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

suo left a comment

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

suo left a comment

Uh oh!

suo May 8, 2020

Uh oh!

SplitInfinity May 8, 2020

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

SplitInfinity commented May 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

suo left a comment

Choose a reason for hiding this comment

Uh oh!

suo May 8, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity May 8, 2020

Choose a reason for hiding this comment

Uh oh!

SplitInfinity commented May 8, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SplitInfinity commented May 8, 2020 •

edited

Loading