Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IndexOutOfBoundsException in NullAway.java #290

Closed
ZacSweers opened this issue Mar 24, 2019 · 5 comments
Closed

IndexOutOfBoundsException in NullAway.java #290

ZacSweers opened this issue Mar 24, 2019 · 5 comments

Comments

@ZacSweers
Copy link
Contributor

Appears to be coming from here:

NullAway/nullaway/src/main/java/com/uber/nullaway/NullAway.java

Line 1289 in 31a1842

ExpressionTree actual = actualParams.get(argPos);

I've filed a more detailed version with a repro diff internally with @lazaroclapp, but filing here for visibility as well

stderr: path/to/File.java:114: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
          .memoryPolicy(MemoryPolicy.NO_STORE)
                       ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.3.1.3-uber
     BugPattern: NullAway
     Stack Trace:
     java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
  	at com.sun.tools.javac.util.List.get(List.java:490)
  	at com.uber.nullaway.NullAway.handleInvocation(NullAway.java:1288)
  	at com.uber.nullaway.NullAway.matchMethodInvocation(NullAway.java:329)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:922)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.visitMemberSelect(TreeScanner.java:680)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:893)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMemberSelect(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCFieldAccess.accept(JCTree.java:2112)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.visitMethodInvocation(TreeScanner.java:508)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:928)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethodInvocation(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCMethodInvocation.accept(JCTree.java:1644)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.visitExpressionStatement(TreeScanner.java:433)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:728)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitExpressionStatement(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCExpressionStatement.accept(JCTree.java:1454)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:533)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1026)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.visitIf(TreeScanner.java:419)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitIf(ErrorProneScanner.java:773)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitIf(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCIf.accept(JCTree.java:1427)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at com.sun.source.util.TreeScanner.visitBlock(TreeScanner.java:248)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:533)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitBlock(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1026)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.visitMethod(TreeScanner.java:206)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:913)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitMethod(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:898)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:90)
  	at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
  	at com.sun.source.util.TreeScanner.visitClass(TreeScanner.java:187)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:593)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:808)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:42)
  	at com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
  	at com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:608)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:147)
  	at com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:591)
  	at com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:56)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:64)
  	at com.google.errorprone.scanner.ErrorProneScannerTransformer.apply(ErrorProneScannerTransformer.java:43)
  	at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:152)
  	at com.sun.tools.javac.api.MultiTaskListener.finished(MultiTaskListener.java:120)
  	at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1404)
  	at com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1353)
  	at com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:952)
  	at com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:100)
  	at com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:142)
  	at com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:96)
  	at com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:90)
  	at com.facebook.buck.jvm.java.plugin.adapter.JavacTaskWrapper.call(JavacTaskWrapper.java:100)
  	at com.facebook.buck.jvm.java.plugin.adapter.BuckJavacTask.call(BuckJavacTask.java:52)
  	at com.facebook.buck.jvm.java.plugin.adapter.BuckJavacTaskProxyImpl.call(BuckJavacTaskProxyImpl.java:134)
  	at com.facebook.buck.jvm.java.Jsr199JavacInvocation$CompilerWorker.lambda$startCompiler$2(Jsr199JavacInvocation.java:421)
  	at com.google.common.util.concurrent.TrustedListenableFutureTask$TrustedFutureInterruptibleTask.runInterruptibly(TrustedListenableFutureTask.java:127)
  	at com.google.common.util.concurrent.InterruptibleTask.run(InterruptibleTask.java:57)
  	at com.google.common.util.concurrent.TrustedListenableFutureTask.run(TrustedListenableFutureTask.java:80)
  	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  	at java.lang.Thread.run(Thread.java:748)
@ZacSweers
Copy link
Contributor Author

Note that the example (.memoryPolicy(MemoryPolicy.NO_STORE)) is from

https://github.com/square/picasso/blob/2.71828/picasso/src/main/java/com/squareup/picasso/RequestCreator.java#L346

and

https://github.com/square/picasso/blob/2.71828/picasso/src/main/java/com/squareup/picasso/MemoryPolicy.java#L23

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Mar 24, 2019 •
edited

Possibly relevant - that API has two parameters - both are the same type, but the second is varag.

memoryPolicy(MemoryPolicy policy, MemoryPolicy... additional)

In this case, only policy is done. But for some reason it's trying to read the additional ones eagerly. If none are actually provided though, perhaps calling get() on that param isn't actually safe in a vararg context since it's optional?

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Mar 24, 2019 •
edited

Looking at the code, I think it's this :)

NullAway/nullaway/src/main/java/com/uber/nullaway/NullAway.java

Lines 1262 to 1265 in 31a1842

if (i == formalParams.size() - 1 && methodSymbol.isVarArgs()) {
// eventually, handle this case properly. I *think* a null
// array could be passed in incorrectly. For now, punt
continue;

@msridhar
Copy link
Collaborator

Thanks, @ZacSweers. I won't have time to look at this for the next few days, but hopefully @lazaroclapp can help.

lazaroclapp added a commit that referenced this issue Mar 25, 2019
@lazaroclapp
Improved varargs support.
4da2895 
Resolves #290 by supporting vararg nullability.

Previously, we ignored the default nullability info of vararg arguments,
but would error out if the vararg formal was deemed non-null by a handler.

Instead, we now correctly acknowledge the nullability of vararg arguments
whether they are non-null due to default code assumptions or due to handlers
(restrictive annotations, `@Contract`, etc).

This PR also adds tests and support for `javax.annotations.Nonnull` which
we were ignoring before and is needed for those tests.
@lazaroclapp lazaroclapp mentioned this issue Mar 25, 2019
Merged
@lazaroclapp
Copy link
Collaborator

lazaroclapp commented Mar 25, 2019 •
edited

Looking at the code, I think it's this :)

NullAway/nullaway/src/main/java/com/uber/nullaway/NullAway.java

Lines 1262 to 1265 in 31a1842

if (i == formalParams.size() - 1 && methodSymbol.isVarArgs()) {
// eventually, handle this case properly. I think a null
// array could be passed in incorrectly. For now, punt
continue;

Indeed it is related. That check was fine, actually (as long as you are ok with ignoring nullability of varargs to avoid the crash), the problem is that there is an else-branch for nullability coming from handlers (e.g. from explicit @NonNull annotations on third-party code) which was missing that check.

Could have just added it to both sides of the branch, but I'd rather just support varargs the right way: #291 ;)

@msridhar Any chance you can give that PR a 5-min sanity review?

lazaroclapp added a commit that referenced this issue Mar 29, 2019
@lazaroclapp
Improved varargs support.
0ce91c0 
Resolves #290 by supporting vararg nullability.

Previously, we ignored the default nullability info of vararg arguments,
but would error out if the vararg formal was deemed non-null by a handler.

Instead, we now correctly acknowledge the nullability of vararg arguments
whether they are non-null due to default code assumptions or due to handlers
(restrictive annotations, `@Contract`, etc).

This PR also adds tests and support for `javax.annotations.Nonnull` which
we were ignoring before and is needed for those tests.
lazaroclapp added a commit that referenced this issue Mar 29, 2019
@lazaroclapp
Improved varargs support. (#291)
401d048 
Resolves #290 by supporting vararg nullability.

Previously, we ignored the default nullability info of vararg arguments,
but would error out if the vararg formal was deemed non-null by a handler.

Instead, we now correctly acknowledge the non-null status of vararg arguments
whether they are non-null due to default code assumptions or due to handlers
(restrictive annotations, `@Contract`, etc).

Note that we don't actually support checking methods taking `@Nullable` var args 
(e.g. we don't support `@Nullable` var args in methods defined in annotated code).
Supporting that would require supporting nullability in generics/array elements, which 
NullAway currently punts on. Instead, we print a corresponding error in this case.

This PR also adds tests and support for `javax.annotations.Nonnull` which
we were ignoring before and is needed for those tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@msridhar @lazaroclapp @ZacSweers