0

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?

Improve this question
10
  • 1
    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 :)) Commented Dec 2, 2020 at 9:17
  • 1
    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. Commented Dec 2, 2020 at 9:21
  • 1
    For reference the password type is: char[] password = null; Commented Dec 2, 2020 at 9:28
  • 1
    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. Commented Dec 2, 2020 at 9:29
  • 2
    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. Commented Dec 2, 2020 at 9:34

2 Answers 2

Reset to default
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.

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

Comments

1

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

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.