8389044: javac crashes with StackOverflowError during type erasure of recursively bounded array type parameter (U extends U[])

[vicente-romero-oracle]

Another crash related to arrays in type variable's bounds

TIA

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Warning

 ⚠️ Found leading lowercase letter in issue title for 8389044: javac crashes with StackOverflowError during type erasure of recursively bounded array type parameter (U extends U[])

Issue

• JDK-8389044: javac crashes with StackOverflowError during type erasure of recursively bounded array type parameter (U extends U[]) (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/32490/head:pull/32490
$ git checkout pull/32490

Update a local copy of the PR:
$ git checkout pull/32490
$ git pull https://git.openjdk.org/jdk.git pull/32490/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 32490

View PR using the GUI difftool:
$ git pr show -t 32490

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/32490.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vromero! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@vicente-romero-oracle The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (59d44be9)
• 00: Full (df52cebd)

[mcimadamore]

I'm starting to get a bit skeptical of all these fixes. I took a look at the code, and there seems to be a timing issue here.

1. We basically first check type var bounds in Attr,attribTypeVariables. This is part of TypeEnter so it runs quite early.
2. After, when a class is attributed, we run Attr.visitTypeParameter, which calls checkIntersectionType -- which is where most errors are reported

The common theme with all these SOE is that we end up doing something bad after (1) but before (2). My feeling is that the check for whether an intersection exists should be done earlier -- possibly in (1). Then if we see that routine reports an error, we just dumb down the type variable bound.

If we don't try to centralize the checks some more, the likely result is that we'll keep uncovering test cases where this timing distinction can be exploited, and we'll end up with several workarounds all over the place.

Now, I'm assuming that, perhaps, running the full checkIntersection is too much -- so some in-place check like the one you have here is probably more pragmatic. My preference though would be for issuing an error immediately if we discover a bad bound (like an array). Then replace the bad bound to use object, or an erroneous type or whatever.

If we do this, IMHO we can revert the fix we did few days ago, because this new fix would probably subsume that.

[mcimadamore]

I think the general fix here is to check whether the head is an array. And if so, report an error and replace it with Object. This should be done regardless of tail. If we do that, I think this can probably generalize the fix we did few days ago (so we can probably revert that one -- but keep the test)

[vicente-romero-oracle]

keeping this condition as just: bounds.head.hasTag(ARRAY) does the job for this bug but we can't remove the fix we did a few days ago as when we do class loading there is a path that stressed the previous fix we did without passing through Attr. Of course this would be exercised when loading a forged class file, not one that would normally be generated by an spec compliant compiler, but still

[mcimadamore]

I believe we have to pick a strategy here. This PR suggests (and I agree) that the best strategy is to sanitize the input before we end up creating weird intersection types. Attr does some sanitization -- but ClassReader will have to do some as well.

[mcimadamore]

I believe we have to pick a strategy here. This PR suggests (and I agree) that the best strategy is to sanitize the input before we end up creating weird intersection types. Attr does some sanitization -- but ClassReader will have to do some as well.

In other words, the best strategy is to add some validation/normalization of the bounds when on-demand completion happens -- whether from source or from classfile. The previous fix attempted to address this at the time an intersection type was created -- which, as this issue reveals, is generally not guaranteed to run in all cases. So I think we'd better add more explicit checks in both completion code paths. One you covered in this PR. The remaining one is ClassReader::sigToTypeParam -- if we see an array bound, we should issue an completion error (e.g. bad classfile). This should cover the waterfront in a more robust way.

Also, we probably need the equivalent of the test in this PR which uses classfile completion instead of source completion.

[jddarcy]

I believe we have to pick a strategy here. This PR suggests (and I agree) that the best strategy is to sanitize the input before we end up creating weird intersection types. Attr does some sanitization -- but ClassReader will have to do some as well.

Does any similar sanitation need to be added to the javax.lang.model APIs?