Comment by sukuriant | Hacker News Reader

Comment by sukuriant | Hacker News Reader

Parent Full thread

Okay. Please tell me I'm reading that wrong.

   void foo(X509Certificate[] a, String unused) {
     if( a != null && a.length == 1) a[0].checkValidity();
     while(true){
        return;
        /* never executed stuff */
        foo(a, unused);
     }
   }

X509Certificate.checkValidity()'s javadoc: http://docs.oracle.com/javase/1.5.0/docs/api/javax/security/...

Selections from above link:

Checks that the certificate is currently valid. It is if the current date and time are within the validity period given in the certificate.

  Throws:
  CertificateExpiredException - if the certificate has expired.
  CertificateNotYetValidException - if the certificate is not yet valid.

There has got to be more to their code than this. It does nothing useful except make sure a random certificate is not expired. If there is no certificate, it doesn't fail or return false or anything. What is this!?

Could this be a profiler-optimized set of code that really had a LOT more code to it? But then why would the bit of random junk after the return statement exist? Did no one do a proper code-review of this???

[edit: I used JavaSE 1.5 above; but I just checked Android's version and it looks to be the same, just less verbage in the docs.]

It looks a lot like the output from an Android decompilation tool... which tends to return some weird stuff. That code segment would never compile (code immediately below a return/break/continue/etc. statement causes a compilation error).

Can you elaborate here? Can you point to an example for some weird Android decompilation?

Just run the dex2jar tool on any android .apk and view the results in jd-gui. I've looked at a few apk's and weirdness like the one noted above is quite common. The code is being reverse engineered from a .smali file and the reverse engineering process appears imperfect. Of course any const names would be stripped away from the decompiled code leaving hardcoded integers (such as the 0 in paramArrayOfX509Certificate[0].checkValidity() ).

I'm not saying that the conclusion of the article is wrong, I'm merely trying to explain why the code blob above looks so weird.

0 is the first element in the array; so, I bet that wasn't a constant, but honestly what the programmer did.

In either case, we have the bytecode; and shouldn't what is written above compile to the same bytecode?

Some code obfuscators will introduce illegal programming constructs. Most VMs will tolerate these but the decompiled source cannot be recompiled without fixing these. Not saying that's what happened here, but it is an option.

So it is a very real possibility that an obfuscation technique that basically throws random bits of code with random extra params in another method and calls them at it would could have been used here. Interesting. It would certainly make more sense.

To demonstrate my point, try writing the following into C.java:

public class C { public int foo() { return 1; int b = 1 + 2; } }

Then compile it (javac C.java). You will get the following:

C.java:4: unreachable statement int b = 1 + 2; ^ C.java:5: missing return statement } ^ 2 errors

Therefore this code is not compileable, thus telling us that the initial code blob could never have run in the first place.

I'm not sure why the decompiler returns non-compiling java code, I just know it does.

That almost hurts to look at -- It looks like a botched attempt at a recursive method to iterate through the array.

I can imagine some poor developer, unable to figure out why it seems to always throw a StackOverflowException, putting that return statement in in frustration, and forgetting about it.

Ouch, that is seriously yucky code.

The issue with the code above is the return statement, most likely inserted during development, with the good intention to remove it later, since the "bit of random junk" that follows would have validated the server's certificate.

But it doesn't. It's calling the exact same function. As your sibling comment remarked, it looked like someone really frustrated with why they were getting a stack-overflow error and gave up.

I'm not sure if things are different with the Android compiler, but javac will not compile code with an unreachable statement after a return like that.

Reply on news.ycombinator.com