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.

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

add diffoscope output for ldapchai-0.8.6.jar #445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

algomaster99
Copy link

@algomaster99 algomaster99 commented Oct 21, 2024

I am analyzing diffs that either show a difference using javap or procyon. One of the first cases I stumbled upon is that the reference and rebuild of ldapchai-0.8.6 jar. In this PR, the main goal is to discuss them. There are three reasons why build is not reproducible. I will put them as comments, but here is summary:

  1. lookupswitch is organised differently.
  2. The order of statements in the constructor differ.
  3. invokeinterface is changed to invokevirtual of .getClass() in the rebuild.

Note that the diffoscope file is extremely truncated. The first commit had 50,000 lines. However, the above three are the major issues shown by procyon and javap. Also, I use a normalization tool jNorm [1] that eliminates basic issues like constant pool ordering and numbering.

[1] Stefan Schott, Serena Elisa Ponta, Wolfram Fischer, Jonas Klauke, and Eric Bodden. Java Bytecode Normalization for Code Similarity Analysis (Artifact). In Special Issue of the 38th European Conference on Object-Oriented Programming (ECOOP 2024). Dagstuhl Artifacts Series (DARTS), Volume 10, Issue 2, pp. 20:1-20:3, Schloss Dagstuhl – Leibniz-Zentrum für Informatik (2024)
https://doi.org/10.4230/DARTS.10.2.20

Comment on lines +30 to +33
│ │ - switch (type.ordinal()) {
│ │ - case 0: {
│ │ + switch (AbstractChaiEntry.AbstractChaiEntry$1.$SwitchMap$com$novell$ldapchai$impl$AbstractChaiEntry$NetworkAddressType[type.ordinal()]) {
│ │ + case 1: {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite weird how the switch condition changes.
At bytecode level, the change is:

-        74: aload_3
-        75: invokevirtual #175                // Method com/novell/ldapchai/cr/HashSaltAnswer$VERSION.ordinal:()I
-        78: lookupswitch  { // 2
-                       0: 104
-                       1: 123
-                 default: 157
+        74: getstatic     #175                // Field com/novell/ldapchai/cr/HashSaltAnswer$1.$SwitchMap$com$novell$ldapchai$cr$HashSaltAnswer$VERSION:[I
+        77: aload_3
+        78: invokevirtual #181                // Method com/novell/ldapchai/cr/HashSaltAnswer$VERSION.ordinal:()I
+        81: iaload
+        82: lookupswitch  { // 2
+                       1: 108
+                       2: 127
+                 default: 161

There seems to a field access, getstatic, which is quite strange.

Comment on lines +283 to +291
│ │ - 1: aload_2
│ │ - 2: putfield #1 // Field val$apacheResponse:Lorg/apache/directory/api/ldap/model/message/ExtendedResponse;
│ │ + 1: aload_1
│ │ + 2: putfield #1 // Field this$0:Lcom/novell/ldapchai/provider/ApacheLdapProviderImpl;
│ │ 5: aload_0
│ │ - 6: aload_1
│ │ - 7: putfield #7 // Field this$0:Lcom/novell/ldapchai/provider/ApacheLdapProviderImpl;
│ │ + 6: aload_2
│ │ + 7: putfield #7 // Field val$apacheResponse:Lorg/apache/directory/api/ldap/model/message/ExtendedResponse;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The statements in the constructor are swapped. Although they are independent of each other, they are still semantically non-equivalent.

-    ApacheLdapProviderImpl$2(final ApacheLdapProviderImpl this$0, final org.apache.directory.api.ldap.model.message.ExtendedResponse var2) {
+    ApacheLdapProviderImpl$2(ApacheLdapProviderImpl this$0, org.apache.directory.api.ldap.model.message.ExtendedResponse var2) {
-     this.val$apacheResponse = var2;     
      this.this$0 = this$0;
+    this.val$apacheResponse = var2;
   }

Comment on lines +705 to +714
│ │ - 45: invokeinterface #49, 1 // InterfaceMethod com/novell/ldapchai/provider/ChaiProviderImplementor.getClass:()Ljava/lang/Class;
│ │ - 50: invokestatic #53 // Method java/lang/reflect/Proxy.isProxyClass:(Ljava/lang/Class;)Z
│ │ - 53: ifeq 76
│ │ - 56: aload_0
│ │ - 57: instanceof #28 // class com/novell/ldapchai/provider/WireTraceWrapper
│ │ - 60: ifeq 76
│ │ - 63: getstatic #27 // Field LOGGER:Lcom/novell/ldapchai/util/internal/ChaiLogger;
│ │ - 66: invokedynamic #59, 0 // InvokeDynamic #1:get:()Ljava/util/function/Supplier;
│ │ - 71: invokevirtual #37 // Method com/novell/ldapchai/util/internal/ChaiLogger.warn:(Ljava/util/function/Supplier;)V
│ │ + 45: invokevirtual #49 // Method java/lang/Object.getClass:()Ljava/lang/Class;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invokeinterface is replaced by invokevirtual. I thought this was a build environment problem, but when I built with jdk=8, I got an error but I did not investigate.

@algomaster99
Copy link
Author

@hboutemy my question is do you think any of these issues are just because of some JVM version inconsistency? Or do you think these are proper semantic differences and the developer should fix them?

@hboutemy
Copy link
Member

do you think these are proper semantic differences and the developer should fix them?

developer used a compiler: he did not do anything for that, I don't see how asking him to fix anything could make anything actionable from his perspective

do you think any of these issues are just because of some JVM version inconsistency?

this would be my fisrt investigation: try rebuilding with many different patch level jdk, and notarizing if bytecode changes at any time when some levels of patch are reached = what I suppose happens: in theory, I'm pretty sure we could track the root cause at some change in javac

@algomaster99
Copy link
Author

algomaster99 commented Oct 23, 2024

he did not do anything for that,

I thought this could be the types of change where the developer could fix by changing the code. For example, consider chains_project.github.io:maven-lockfile:3.4.0. One of its artifacts, maven-lockfile-github-action-3.4.0.jar is non-reproducible because its dependencies generates a lot of code at build time which is non-deterministic in nature.

I'm pretty sure we could track the root cause at some change in javac

Okay, validates my reasoning that this is the fault of .buildspec configuration. I will look into this.

@hboutemy
Copy link
Member

hboutemy commented Nov 6, 2024

@algomaster99 2 interested cases for you:

  1. xmlsec 3.0.5: GHA rebuild produces different bytecode from local build (same Azul JDK 1.8.0.432) https://github.com/jvm-repo-rebuild/reproducible-central/actions/runs/11700115769/job/32583453280 , no idea how it is possible
  2. bytebuddy: require local build, GHA gets different result: I thought it was based on precise JDK version, perhaps GHA has an impact like xmlsec; I think I tested local build with different JDK patch levels and got different releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants