Some subtle and not-so-subtle Java bugs
By Erling Ellingsen on August 18, 2006One
Why is this wrong?
public static String escapeSql(String str) {
if (str == null) {
return null;
}
return StringUtils.replace(str, "'", "''");
}
(source: Apache Commons "lang" package)
Two
There are at least
package com.initech.web.fluff;
public class StringUtils {
private static final SimpleDateFormat UTC_FORMAT = new SimpleDateFormat("yyyyMMdd'T'HH:mm:ss");
public String formatUtcDate(Date d) {
return UTC_FORMAT.format(date);
}
}
(source: lots)
Three
This code is supposed to protect the 'override' flag from being set if 'obj' is a constructor for the 'Class' class. Can you set it anyway, without using reflection? "Three" is a hint, by the way.
class AccessibleObject {
private boolean override;
/* Check that you aren't exposing java.lang.Class.<init>. */
private static void setAccessible0(AccessibleObject obj, boolean flag)
throws SecurityException
{
if (obj instanceof Constructor && flag == true) {
Constructor c = (Constructor)obj;
if (c.getDeclaringClass() == Class.class) {
throw new SecurityException("Can not make a java.lang.Class" +
" constructor accessible");
}
}
obj.override = flag;
}
(source: JDK 1.5)
TrackBack
TrackBack URL for this entry:
http://blog.medallia.com/cgi-bin/mt/mt-tb.cgi/9
Comments
Reinier:
1: Agreed, j.sql.PreparedStatement would be the Right Thing here.
Assuming the user adds single quotes around the "escaped" string himself, how would you inject SQL? (the method I was thinking of works with Mysql and Postgres, not sure how other dbs handle this)
2: Oops, the missing 'static' and dashes was unintentional. I have updated the post to include those two in the count :-)
3: Your "random sidenote" is close...
Not quite sure how '3' is a hint, but I get it now:
pass Boolean true to the function instead of boolean true. == auto-boxes the second argument and the resultant comparison fails: (new Boolean(true) == Boolean.valueOf(true)) would be false, and .valueOf is how java internally autoboxes.
java 1.5. Yummy source of puzzlers.
That was it right? Score one?
for #1: Did I get it?2 rants on #1. Feel free to skip on.
rant #1: Proper style. null is just not going to work in the above example, and it cannot be fixed: Because the caller is responsible for adding the quotes, you end up with a quoted 'null' either way - whether you return the string null (sans quotes) or the actual null value, which gets translated to the string null when concatenating strings.
Add to that the disastrous side-effect of forgetting to manually add the quotes, security wise, and the story is complete:
Letting the caller add the outside quotes is Wrong, with a very large capital W.
rant #2: Security.
I don't know how to circumvent that escapeSql method. That's the point. I am missing details. Maybe some intermediate layer somewhere tries to be clever and attempts to translate certain unicode characters down to the nearest ASCII equivalent, allowing a character that java does not identify as a quote end up as a quote ANYWAY.
Maybe the database parser has an internal escaping device, like java's own backslash u.
Maybe inserting 'oogieboogieboola' into the string mysteriously accesses a little known backdoor.
The point is this: It depends on the underlying dbase AND version. No way this particular bit of code is intended to be married that explicitly to the database.
This kind of thing is why PHP has awkwardly named functions like pg_sql_escape and why java has the PreparedStatement.
You -always- use them. Even if for all mainstream database engines, all versions, that escapeSql method can be engineered and proven to work reliably... it would still be Wrong.
Done with the rants. So, going back to number one, and to indulge your question: Did I actually get it?Which leaves number 2. I mentioned my doubts regarding the safety of using Date in combination with something that is apparently timezone sensitive. Then again Date is supposed to be exclusively UTC. Convenient.
How many for #2 did I miss?
#1 has been completely slaughtered. With your permission I'd like to use your rant above for the "solutions" post :-)
#2: I had two and you found three :-) There's just one (not very exciting) bug left. The symptoms for this one are mysterious, irreproducible ArrayIndexOutOfBoundsExceptions and occasional wrong results.
#3: Ah, but since the parameter is a primitive boolean, the Boolean will be auto-unboxed (by calling booleanValue()) by the caller. It is not a 1.5 bug!
By all means, use whatever you need.
I'll think a bit longer on #2.
As far as #3 goes: Natch, you're right. Damn. I give up for now.
Friend of mine also likes java puzzlers a lot. He'll be back on monday. I wonder how far he'll get.
I found the fouth bug in the second assignment.
SDF isn't synchronized - calling it in 2 threads bollockses up both results.
Proposed solution: Either synchronize it, or for thread pool situations, use a ThreadLocal to store the SDF instance.
That just leaves that pesky 3...
... I give up. Can't figure out #3,
answers below.....
1. I'm guessing the problem lies with intention. One of these 2 is what you're on to, I think:
1a) null itself should be escaped to the local SQL equivalent - the string "NULL"... though for equal checks this would fail again depending on dbase implementation (ie, in postgres, NULL == NULL would return false, NULL IS NULL is true).
1b) That's not good enough to escape strings. At least put quotes around the outside too, and that might not even be enough. JDBC allows you to put in question marks, and then replace those question marks with prepared strings, which are guaranteed to be 'safed' against SQL injection attacks.
2.
a. According to http://www.w3.org/TR/NOTE-datetime, that's not UTC format. Add the timezone at the end, and separate out the day, month, and year with dashes.
b. timezone sensitive stuff really ought to go with Calendar. If that isn't needed, the standard millis since the epoch (long) format serves best, not an instance of the Date class.
c. given the class name and the independence of the method code to any attributes, the method should probably be declared static.
3. Hmm, I don't think I came up with what you were looking for, bit, fwiw:
a. If obj is a Constructor object for the Class class but for a Class class that has been loaded by another class loader, then the one Class does not equal the other. The only practical way to check that would be with a string match to 'java.lang.Class' which is rather ugly.
b. I get the feeling that the == comparison between obj's declaring class and Class.class is a problem, and .equals() should be used instead. I'm certain using equals() won't hurt, but perhaps in this particular case == instead of equals() is irrelevant.
c. Any constructor of a subclass of Class passes the test. Of course, Class is final, so it would take hacking the runtime library.
(random sidenote: there's a boolean == true comparison in there. That's ridiculously superfluous.)
None of these are very satisfying, I'm afraid.
How'd I do?