Summary: | ICC profile management can lead to a crash due to lack of reference counting of profiles | ||
---|---|---|---|
Product: | Ghostscript | Reporter: | Ken Sharp <ken.sharp> |
Component: | Color | Assignee: | Michael Vrhel <michael.vrhel> |
Status: | RESOLVED FIXED | ||
Severity: | normal | ||
Priority: | P4 | ||
Version: | master | ||
Hardware: | PC | ||
OS: | Windows 10 | ||
Customer: | Word Size: | --- |
Description
Ken Sharp
2020-09-10 14:19:54 UTC
Ken, So I am confused why there is a decrement of the icc structure in that code. The code that is called does not create a new icc structure if there is currently one set for the device. It does update the ICC profile(s) in the structure. What is : case ccs_RGB: /* Only bother to do this if we didn't handle it above */ if (!pdev->params.ConvertCMYKImagesToRGB) { if (pdev->icc_struct) rc_decrement(pdev->icc_struct, "reset default profile\n"); pdf_set_process_color_model(pdev,1); ecode = gsicc_init_device_profile_struct((gx_device *)pdev, NULL, 0); if (ecode < 0) goto fail; } break; trying to accomplish? We could do a decrement and then set it to NULL like you did, or not do the decrement. Is there an rc_increment lurking someplace in the pdfwrite code that occurs in another case? (I could not find one). (In reply to Michael Vrhel from comment #1) > So I am confused why there is a decrement of the icc structure in that code. > The code that is called does not create a new icc structure if there is > currently one set for the device. It does update the ICC profile(s) in the > structure. > > What is : > > case ccs_RGB: > /* Only bother to do this if we didn't handle it above */ > if (!pdev->params.ConvertCMYKImagesToRGB) { > if (pdev->icc_struct) > rc_decrement(pdev->icc_struct, > "reset default profile\n"); > pdf_set_process_color_model(pdev,1); > ecode = gsicc_init_device_profile_struct((gx_device *)pdev, > NULL, 0); > if (ecode < 0) > goto fail; > } > break; > > trying to accomplish? Its changing the ProcessColorModel of the device from whatever it currently is to the device colour space specified by the ColorConversionStrategy. So that if we render anything (eg transparent objects) they will be rendered in the colour space that the user requested. > We could do a decrement and then set it to NULL like you did, or not do the > decrement. Is there an rc_increment lurking someplace in the pdfwrite code > that occurs in another case? (I could not find one). There are plenty of increments around, not all direct (eg gsave/grestore), but that's not the problem. My only question is what is the correct/normal/desired way to deal with this situation ? Is it better to decrement the reference count and NULL the object pointer or not decrement the current profile. Obviously its shall we say 'unusual' for a device to change its ProcessColorModel, but the pdfwrite family can do so. Its been a couple of months since I looked at this, so my memory is hazy, but as I recall each possibility worried me because the ICC profile is being shared between the two devices. I'm assuming that this is because the two devices have the same ProcessColorModel and that if they did not then they would not share the profile via the icc_struct but I couldn't prove that. In the example I quoted I believe that there is no actual change in the colour model, its changing from RGB to RGB and arguably we should optimise that out, but I was concerned there is a general problem if we switch devices and the colour model changes. Do the devices continue to share an ICC profile via icc_struct ? I'm not convinced that its not possible to end up in this situation and genuinely be altering the colour model of the device. So I would say, do not do the decrement. You are not replacing the ICC structure. It is a container that holds possibly multiple profiles. Those profiles will be updated if needed in gsicc_init_device_profile_struct and any appropriate ref counting performed there. The fact is the ICC profile(s) in the ICC structure could be referenced elsewhere. If a profile is replaced in the ICC structure, the old one has its rc decremented. If some other device is sharing this icc struct (which seems weird to me -- but the use of the NULL device for this case seems weird to me) then it will have its ICC profile updated also. Fixed in commit e6d34e7d08dac91b5b3b858c23e186a0d3bcbefc |