8055461: getNextID in ImageIcon class can lead to overflow #25666
Conversation
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| width = image.getWidth(imageObserver); | ||
| height = image.getHeight(imageObserver); | ||
| return; | ||
| } | ||
| mTracker.addImage(image, id); |
There was a problem hiding this comment.
Hmm. Is there any acceptable logging we could use here?
(I'm just thinking: from the perspective of a developer trying to debug a customer complaint, this would be a lot easier to identify in System.err mentioned it...? Otherwise I'd start by trying to look for potential memory leaks or other red herrings.)
There was a problem hiding this comment.
I dont think there is any logging for this nor do we want to log...In all probability, the image will consume the heap memory so practically OOM will be raised by the time this overflow happens but it can happen if maybe all 1x1 images are used and subsequent needed memory reserved for Java process
There was a problem hiding this comment.
OK. I don't feel like I really understand this fix (or the need for it), but I also am not too worried about it.
FWIW I was able to trigger this condition in an app that ran for about 30 minutes. (Maybe if I turned up the thread priority it could be faster?) I'm not going to keep exploring this, though, unless a pointed question comes up.
import javax.swing.*;
import java.awt.*;
public class ImageIconTest {
public static void main(String[] args) {
SwingUtilities.invokeLater(new Runnable() {
@Override
public void run() {
JFrame f = new JFrame();
ImageIcon icon = new ImageIcon();
Image image = Toolkit.getDefaultToolkit().createImage("onepixel.gif");
icon.setImage(image);
JLabel label = new JLabel(icon);
f.getContentPane().add(label);
f.pack();
f.setVisible(true);
Thread t = new Thread(new Runnable() {
@Override
public void run() {
while (true) {
icon.setImage(image);
}
}
});
t.start();
}
});
}
}
There was a problem hiding this comment.
trigger this condition in an app that ran for about 30 minutes.
What condition? I couldnt get OOM even after running several hours in Windows and Mac...id didnt reach 2147483647
This issue was not raised by me so clearly somebody felt the need for it..Also, I think it is better to get ABORTED status for the image to let the user know so that corrective action can be taken,
instead of aborting the application with OOM or whatever exception it gets thrown
There was a problem hiding this comment.
What condition?
I just meant the int overflow condition. That test is not meant to demonstrate an OOM.
I understand why an OOM feels related to this ticket (and maybe that really is the compelling concern here), but I don't think that ticket mentioned memory consumption.
Here's a recording that (I think?) shows I reached the int overflow in about 15 minutes:
https://go.screenpal.com/watch/cT16h0nX6xx
(You may need to look at the system clock in top-right to accurately track time; parts of the video are sped up 10x. It should be pretty clear when the text cursor blinks super fast.)
We could expand that test to:
A. use an animated gif, or
B. try to load a new non-trivial image once we cross into a negative ID
... but that circles back around to the broader "what are we really trying to solve" question. (And I don't think I know the answer to that question.)
This issue was not raised by me so clearly somebody felt the need for it.
Sure.
Also, I think it is better to get ABORTED status for the image to let the user know so that corrective action can be taken, instead of aborting the application with OOM or whatever exception it gets thrown
OK. This feels too theoretical for me to invest strongly in a recommendation.
On the one hand: what you're saying makes sense, and obviously avoiding an OOM is a good thing to do.
On the other hand: if the integer overflow occurs in a non-trivial real-world usage: I'm guessing the images/icons already are actually being responsibly disposed of/garbage collected over time. So what we really need is to reset the counter or use a new MediaTracker if the int overflow condition is reached. For example, suppose we use all 2147483647 images IDs, and each image is 100x100px and 3 color channels. That's going to require about 60,000 GB of data (in terms of pixels). If those images are kept in memory, then we will reach an OOM long before we reach an integer overflow. So the fact that we get here (to the integer overflow) at all may (?) mean the application is responsibly discarding images and it's able to handle thousands more over time.
Most of all I'd like to see a real-world use case explaining how this impacts a user. Until then: this PR looks good/harmless (IMO).
There was a problem hiding this comment.
OK. This feels too theoretical for me to invest strongly in a recommendation.
That's why I mentioned in the description itself
Theoretically there is a possibility that there can be overflow in the long time run or for large number of created "imageIcon"
There was a problem hiding this comment.
What's the deal with OOM at all?
If the application doesn't keep all the images it loads, no OOM will occur. Throwing OOM has nothing to do with the stated problem.
| if (id < 0) { | ||
| loadStatus = MediaTracker.ABORTED; | ||
| width = image.getWidth(imageObserver); | ||
| height = image.getHeight(imageObserver); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This means that the application won't be able to load images using MediaTracker, or rather create new ImageIcon instances.
Is it really a problem that the id becomes negative? Does anything stop working?
I can't see that anything breaks because of that. The id is just an id, and it's compared for equality. (Although having a negative id could look suspicious.)
It may break at MediaEntry insertion, although it would still work probably…
Another problem may occur if and when the id wraps around again to positive numbers and 0, 1… are still being used.
| width = image.getWidth(imageObserver); | ||
| height = image.getHeight(imageObserver); | ||
| return; | ||
| } | ||
| mTracker.addImage(image, id); |
There was a problem hiding this comment.
What's the deal with OOM at all?
If the application doesn't keep all the images it loads, no OOM will occur. Throwing OOM has nothing to do with the stated problem.
If we create a new MediaTracker in ImageIcon and start anew with id of zero, then if application is having its own MediaTracker to track then when it calls for tracker.checkID(0) or tracker.statusID(0) then it will be confusing to which image it is referring to, Other thing that can be done is to modify "int" to "long" for all ids to further the overflow goalpost whereby |
Why is it? If an application needs to load images, it has to create its own instance of
Could you elaborate please? The This means if
I thought about it, and using But it won't solve the underlying problem: |
WHy application cant use new ImageIcon I didnt understand? |
|
Something like this in this case wont mediaTrackerId starts from 0 again for "newIcon"? |
No, it won't. Why will it?
jdk/src/java.desktop/share/classes/javax/swing/ImageIcon.java Lines 322 to 326 in bf7d40d With the change that you propose, it will be impossible to load images with |
|
I wonder why JDK-8055461 is an enhancement. It's a bug. |
OK.
So, we need to have mediaTrackerID1 and use that but then what about when next Integer.MAX_VALUE is reached, create another MediaTracker and again use another mediaTrackerID2 ? it will be a loop then... |
No, just one. When |
Likely no application loads more than 2 billion images; it's even less likely that an application will load more than 2⁶⁴ images which is closed more than 10¹⁹. In fact, it seems to me that nothing will actually break when the integer counter in Because of negative id, the order of jdk/src/java.desktop/share/classes/java/awt/MediaTracker.java Lines 884 to 887 in c98dffa It may result in malfunctioning… or not. If not, nothing bad will happen. Having said that, I think that no fix is necessary. Yes, A problem would occur if and only if an image with id of 1, 2… still exists in the |
If mediaTrackerId overflows then id will increment backwards without any fix like |
|
I've been trying to verify whether the fix is even necessary. My test case has been running for a few hours, and it reached only 0x30000000. I will need to update the test case to cause the overflow from negative number to positive again. Let's see. I still like the idea of not touching anything better than fixing this potential issue which doesn't seem to cause much trouble, at least from analysing the code. |
|
Here's the test case that I used. (Thanks to @mickleness for the code, setting an image significantly increased the performance of the test; my initial test loaded an image from the disk for each instance of
|
|
But if the fix doesnt hamper anything and still fixes the problem of overflow (even if overflow itself doesn't apparently cause any issue) maybe we should consider it.. |
The fix makes the code more complicated for no reason because… in your own words
Exactly! If it ain't broken and it works, don't fix it.
We may consider it… Let's see if anyone else has an opinion. Yet I'm strongly inclined to not touching it because it works as expected even when |
|
Interesting that the MediaTracker is managed via an AppContext, but the id a static field. I don't see using a long as helping because aside from being able to still wrap around you I don't see a problem with a negative ID. MediaTracker doesn't require a positive value. I think that stalling is an unlikely problem. |
ImageIcon.getNextID uses
mediaTrackerIDwhich do not detect overflow.Theoretically there is a possibility that there can be overflow in the long time run or for large number of created "imageIcon"
Made sure there is no overflow and treat that loadImage as ABORTED
No regression testcase as it addresses theoretical possibility..
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25666/head:pull/25666$ git checkout pull/25666Update a local copy of the PR:
$ git checkout pull/25666$ git pull https://git.openjdk.org/jdk.git pull/25666/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25666View PR using the GUI difftool:
$ git pr show -t 25666Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25666.diff
Using Webrev
Link to Webrev Comment