Community
Participate
Working Groups
Methods ProblemHandler.handle(..) take care to null-out the field referenceContext at the end, but some methods in ProblemReporter do not invoke handle(..) and thus this reference may leak to later invocations. Although I don't have a test case at hand, I think this could produce results like this: - a method with bogus javadoc could trigger javadocMissingParamTag(..) - severity is configured to ignore so reporting exits early -> referenceContext remains pointing to this MethodDeclaration - later a different method refers to a binary type, which is built with cachePartsFrom(..), but during this process it hits, e.g., corruptedSignature(..). - here we have no referenceContext but the previously leaked one will be reused during error reporting. Depending on the exact scenario this could cause: - reporting of an error against an unrelated class - reporting of an error against the current class that normally would be reported against the project (i.e., no source location)
Jesper, please see if the recent defensive change discussed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=393192#c19 adequately covers this and if so resolve this bug. TIA. (lower priority than other than tasks you have, but can be knocked off when cycles become available)
Normal passes of the compiler will not see this a problem. I reproduced this via a tainted JAR file and disabled JavaDoc warnings, but there's one little detail: Every time new method binding is created by MethodScope, it is set as new referenceContext, so when BinaryTypeBinding finds the error in the class file, referenceContext points somewhere reasonable already. Attaching test case for c0 which actually passes acceptably
Created attachment 228237 [details] Test case Perhaps should be in a different class, but since the actual issue is with the JavaDoc problem reporting, it felt acceptable.
Created attachment 228238 [details] Test JAR to add into "org.eclipse.jdt.core.tests.compiler/workspace" directory (Source and instructions on tainting is in the jar)
(In reply to comment #4) > Created attachment 228238 [details] Questions: (1) Does this already contain the tainted version of the class file ? (2) What is this test proving ? On BETA_JAVA8 where the fix for resetting the reference context as discussed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=393192#c19 is not there (not cherry picked yet), this test passes. We would be ready to resolve this bug only if we are sure the resetting done at the end of Parser.parse() adequately covers the problem reported in comment#0 - This is not clear to be the case from the tests devised so far ?
(1) Yes, the .class file in the attached JAR is tainted (2) The test shows that due to other side-effects (e.g. each new method re-setting the reference context), the scenario outlined in c0 does not pose a problem. The test also passes in master. As I see it, the only 100% safe solution would be to (A) pull the reference context out of the ProblemReporter, and explicitly pass it into each method - but that takes massive rework. The less safe (B) solution is to make it an implementation requirement of the individual methods of ProblemReporters to clear the referenceContext, if they do not call handle(). This is a medium task, and could be forgotten by someone in the future. Finally, the status quo (C) is to keep this potential pitfall in mind when strange errors like bug 393192, and clear it when exiting the parser, so at least it's in a consistent state there.
(In reply to comment #6) > The less safe (B) solution is to make it an implementation requirement of > the individual methods of ProblemReporters to clear the referenceContext, if > they do not call handle(). This is a medium task, and could be forgotten by > someone in the future. Sorry for not being clear: My question is whether the clearing of the reference context in Parser.parse adequately implements (B). OIOW those locations that don't call handle today, are they all originating while Parser.parse is on the call stack.
(In reply to comment #7) > (B). OIOW those locations that > don't call handle today, are they all originating while Parser.parse is on the > call stack. No, not all of them, at least not theoretically -- one example is: ProblemReporter.assignmentHasNoEffect can be suppressed, but CAN be called through TypeDeclaration.generateCode > ClassFile.addFieldInfos > addFieldInfo > addFieldAttributes > FieldBinding.constant > FieldDeclaration.resolve(MethodScope). That's just an example, and I don't know if it is possible in practice OR if it warrants specific action.
(In reply to comment #8) > (In reply to comment #7) > > (B). OIOW those locations that > > don't call handle today, are they all originating while Parser.parse is on the > > call stack. > > No, not all of them, at least not theoretically -- OK, thanks, this bug can be worked on opportunistically when time permits at a lower priority than the others you have.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
