Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultUpdateHandler heap space exception when file to download is 0b #106

Closed
yildiz-online opened this issue Sep 6, 2020 · 8 comments
Closed

Comments

@yildiz-online
Copy link

yildiz-online commented Sep 6, 2020

Due to a bug in my app, the update4j xml generated ended up with a 0b file size instead of correct size(65mo), leading to this exception in the client using that xml:

Part of the XML file:

file uri="http://files.yildiz-games.be/play50hz/player/retro-player.jar" path="${user.dir}/lib/retro-player.jar" size="0" checksum="1"

Exception:

Downloading 0 B 0% [> ] Exception in thread "Progress Printer" java.lang.OutOfMemoryError: Java heap space
at java.base/java.util.Arrays.copyOf(Unknown Source)
at java.base/java.util.StringJoiner.add(Unknown Source)
at java.base/java.lang.String.join(Unknown Source)
at org.update4j.util.StringUtils.repeat(StringUtils.java:123)
at org.update4j.service.DefaultUpdateHandler.renderProgress(DefaultUpdateHandler.java:189)
at org.update4j.service.DefaultUpdateHandler$1.run(DefaultUpdateHandler.java:127)
at java.base/java.util.TimerThread.mainLoop(Unknown Source)
at java.base/java.util.TimerThread.run(Unknown Source)

Same file with expected size(65mo) is handled correctly.

@mordechaim
Copy link
Contributor

mordechaim commented Sep 6, 2020

Thanks, I'll look into this.

Does this happen on JDK 11+ too?

@yildiz-online
Copy link
Author

yildiz-online commented Sep 6, 2020

Sorry forgot to mention env,
java 11, win64

@mordechaim
Copy link
Contributor

mordechaim commented Sep 6, 2020

Sorry forgot to mention env,
java 11, win64

The env usually doesn't matter but in the case of String::repeat I created a polyfill to work on earlier versions. It seems to be a bug in the core JDK.

I'll look into it when I have a chance.

@mordechaim
Copy link
Contributor

mordechaim commented Sep 6, 2020

I have a hunch is an int overflow bug.

@mordechaim
Copy link
Contributor

mordechaim commented Sep 7, 2020

The problem is here:

handler.updateDownloadFileProgress(file, (float) (currentCompleted / file.getSize()));

Which is a divide by zero. While for integers is fails with an ArithmeticException it will silently pass for real numbers with value Float.POSITIVE_INFINITY.

Then in this line:

int pieces = (int) ((progressWidth - 2) * currentFrac);
String line = repeat(pieces, "=");

we multiple an int by Infinity which results in Integer.MAX_VALUE.

Attempting to repeat a string by max value will cause an OutOfMemoryError

@mordechaim
Copy link
Contributor

mordechaim commented Sep 7, 2020

I think this a very special case that should probably not be handled by update4j. It will fail regardless in the file validation phase.

@yildiz-online
Copy link
Author

yildiz-online commented Sep 8, 2020

Totally agree that it should fail (input is wrong anyway) but would be nice to have a better suited exception thrown early to avoid filling the heap space .

Or maybe ignore files with size 0 and checksum 1 ?

@mordechaim
Copy link
Contributor

mordechaim commented Sep 8, 2020

The safest and least intrusive would be to simply clamp the progress between 0 and 1 and never allow leaking out of those bounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants