Java code efficiency when converting string [duplicate]

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
– Alex Shesterov, Commented Dec 2, 2020 at 9: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
– Ivar, Commented Dec 2, 2020 at 9:21
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
– Amongalen, Commented Dec 2, 2020 at 9: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
– Ralf Kleberhoff, Commented Dec 2, 2020 at 9:34

kutschkem · Accepted Answer · 2020-12-02 09:40:49Z

1

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.

answered Dec 2, 2020 at 9:40

kutschkem

8,2133 gold badges26 silver badges66 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Joop Eggen · Accepted Answer · 2020-12-02 10:34:10Z

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, ' ');

Collectives™ on Stack Overflow

Java code efficiency when converting string [duplicate]

2 Answers 2

Comments

Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Linked

Related