Java code efficiency when converting string

Question

I have decompiled a fix provided by a 3rd party development team.

This is the original code:

if (this.getPassword() != null) {
    this.uid = this.getUserName();
    password = this.getPassword();
}
if (this.host != null) {
    en.put("hostname", this.host);
    if (password != null && password.toString().trim().length() != 0) {
        en.put("password", password.toString());
    }
    if (this.uid != null && this.uid.trim().length() != 0) {
        en.put("userID", this.uid);
    }
}

and this is the fix:

if (this.getPassword() != null) {
    this.uid = this.getUserName();
    final char[] passwordTemp = this.getPassword();
    password = new char[passwordTemp.length];
    for (int i = 0; i < passwordTemp.length; ++i) {
        password[i] = passwordTemp[i];
    }
}

if (this.host != null) {
    en.put("hostname", this.host);
    if (password != null && password.toString().trim().length() != 0) {
        en.put("password", new String(password));
    }
    if (this.uid != null && this.uid.trim().length() != 0) {
        en.put("userID", this.uid);
    }
}

This seems to have significantly degraded the performance of the code. Does anyone know why this would be done? and is there a better way to do this?

See linked question. They've used `char[]` instead of `String` for passwords. However, their implementation is very poor, as the password is still converted to string later :)) — Alex Shesterov, Dec 02 '20 at 09:17
I assume that the characters in the array returned by `this.getPassword()` are erased at some point, while it is still needed elsewhere. Therefore they copy the password to a different array that wont be destroyed. — Ivar, Dec 02 '20 at 09:21
Thanks, it does make sense, however, is it necessary to use the FOR loop to populate the password variable and is this inefficient? is there a better way of doing it that is more efficient? — bazza2000, Dec 02 '20 at 09:25
After looking at your code I have to admit I'm quite confused. If `password` is `char[]` then calling `toString` on it is quite pointless, you won't get String representation of characters in the array. — Amongalen, Dec 02 '20 at 09:29
Have you profiled the application after the change? Is the bottleneck really in the code snippet where you're suspecting it? I've learned the hard way that speculation about performance issues nearly always fails. — Ralf Kleberhoff, Dec 02 '20 at 09:34
@Amongalen That means the check for empty password is broken, right? — kutschkem, Dec 02 '20 at 13:30
@kutschkem it seems so. What is more, in the first example `password.toString()` is added to the map so the whole login mechanism won't work most likely. — Amongalen, Dec 02 '20 at 13:44

score 1 · Answer 1 · answered Dec 02 '20 at 09:40

What they do here is copy the password array. The probable reason why: They want to ensure that the password is not changed in the original object. In the earlier version, they leak the reference to the password array to some part of the code where they may or may not change the password.

Either there was a real bug where the password got changed but shouldn't, or they just want to ensure that this kind of bug can't happen later on.

Note they wouldn't need this if they worked with Strings instead of char arrays because Strings are (meant to be) immutable.

Joop Eggen · Answer 2 · 2020-12-02T10:34:10.807

I do not see a significant performance improvement besides doing checks earlier.

The following is just better style:

            password = new char[passwordTemp.length];   
            for (int i = 0; i < passwordTemp.length; ++i) { 
                password[i] = passwordTemp[i];  
            }

could be written as:

            password = Arrays.copyOf(passwordTemp, passwordTemp.length);

It might improve performance as the JIT might have more reason to compile copyOf.

In new Java as of version 11 one may use isBlank. There was still a problem in the old code. Password checked trimmed (not array's toString) and maybe storing it trimmed to prevent troubles.

        if (password != null && new String(password).trim().isEmpty()) { 
            en.put("password", new String(password));   
        }

with isBlank:

        if (password != null && !new String(password).isBlank()) { 
            en.put("password", new String(password).trim());   
        }

(Two new's can be optimized.)

The entry in the map ideally should be a char[] and the optionality in the map en is unfortunate. But that was not your code.

For security relevance (SonarQube, security screening) you might do at the end:

...
en.put("password", "");
Arrays.fill(password, ' ');

Java code efficiency when converting string

2 Answers2