TrustAllX509TrustManager lint check incorrectly flags interfaces that extends X509TrustManager [270065082]

Fixed

Bug

Status Update

No update yet.

Description

ry...@intune.corp-partner.google.com

created issue #1

Feb 21, 2023 04:16PM

DESCRIBE THE ISSUE IN DETAIL:

STEPS TO REPRODUCE: 1.Create an interface that extends X509TrustManager

public interface ExtendedX509TrustManager extends X509TrustManager {
    List<X509Certificate> checkServerTrusted(X509Certificate[] chain, String authType, String host)
            throws CertificateException;
}

Extend the new interface correctly implementing checkServerTrusted and checkClientTrusted.
run gradlew lintDebug

EXPECTED: lint should pass ACTUAL: lint flags ExtendedX509TrustManager interface for not implementing checkServerTrusted or checkClientTrusted even though it is just and interface. The classes that fully implement the interface are not flagged (as expected).

ATTACH SCREENSHOTS/RECORDINGS OF THE ISSUE

ATTACH LOG FILES (Select Help > Show Log in Files, or Show Log in Finder on a Mac)

IMPORTANT: Please read https://developer.android.com/studio/report-bugs.html carefully and supply all required information.

Studio Build: Version of Gradle Plugin: Version of Gradle: Version of Java: OS:

Comments

ry...@intune.corp-partner.google.com <ry...@intune.corp-partner.google.com> #2Feb 21, 2023 04:20PM

I can reproduce this (thanks for the repro project!)

It looks like the problem is that the desugared api list from r8 contains this entry:

java/util/Collection#removeIf(Ljava/util/function/Predicate;)Z

but the bytecode here doesn't match -- it's java/util/ArrayList. Collection isn't a directly implemented interface or a direct super class, it's an interface on the super super class. The most efficient thing runtime wise would be for the signature list to inline this method on all implemented subclasses. But I should probably at least for now go and make the desugared API lookup do something similar to what it does for API lookup -- search through all super classes and interfaces as well. This isn't a new problem, so I'm very surprised this hasn't come up before (or it has, and I've forgotten).

ha...@google.com <ha...@google.com> Feb 21, 2023 06:22PM

Assigned to an...@google.com.

tn...@google.com <tn...@google.com> Feb 23, 2023 08:26PM

Reassigned to tn...@google.com.

tn...@google.com <tn...@google.com> #3Feb 24, 2023 10:59PM

Marked as fixed.

(I have a pending CL that was working to improve the handling of fields now that r8 handles desugaring fields, I'll try to dust that off and combine the fix in there.)

da...@robinhood.com <da...@robinhood.com> #4Aug 9, 2023 04:21PM

I went to implement this, and hooked up inheritance search when analyzing the source file containing the call.

However, lint also handles the case where the library being analyzed is not using core library desugaring (for example, it may be a plain Java library). But when that library is consumed in a downstream app module, where library desugaring is turned on, lint then processes the partial results from the library and filters each violation through the desugaring allowlist.

At this point, it's tricky to do the inheritance search -- this happens when we no longer have a compilation environment and can do class inheritance lookups. So there are three possible solutions.

First, we pay the cost up front -- even when you're not using core library desugaring, we record whether the method is potentially library desugared if turned on. (This is also tricky because at this point we don't know which exact desugaring library version is used, which determines the exact list of APIs).

Or, more expensively, for every API violation of this type we store all the potential super class and interface names for each result...

Or, we handle this in the code which generates the desugaring API list, inlining all subclasses affected. This could be quite a long list, but on the other hand this list is really only intended to be machine readable.

Issue 270065082