Bug 702885 - ICC profile management can lead to a crash due to lack of reference counting of profiles
Summary: ICC profile management can lead to a crash due to lack of reference counting ...
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Color (show other bugs)
Version: master
Hardware: PC Windows 10
: P4 normal
Assignee: Michael Vrhel
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-10 14:19 UTC by Ken Sharp
Modified: 2020-11-04 09:11 UTC (History)
0 users

See Also:
Customer:
Word Size: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ken Sharp 2020-09-10 14:19:54 UTC
The pdfwrite device (but potentially other devices) can be induced to seg fault by switching devices to the nulldevice and back again, under certain conditions.

Using this command line:

gs -sDEVICE=pdfwrite -dPDFSETTINGS=/ebook -o out.pdf input.ps

and this PostScript (extracted form the CET test file 09-11.ps):

   /before_setpagedevice currentpagedevice def
   gsave
   nulldevice
         <<
            /Password 0
            /PageSize [ 792 612 ]
            /ManualFeed true
            % One device gets invalidaccess if Policies exists
            % during the first call, when nulldevice is active.
            % That seems anomalous, so I am keeping the test so:
            /Policies
            <<
               /PolicyNotFound 1
               /PageSize 1
               /ManualFeed 1
            >>
         >>
         setpagedevice
   grestore

The cause is sharing of ICC profiles between 'icc_struct' instances.

When we set the nulldevice, it 'inherits' the icc_struct from the current (pdfwrite) device. At this juncture icc_struct->device_profile[0] is pointing to the default RGB profile. Sharing the icc_struct increments its reference count to 2.

For reasons best known to Ghostscript we then execute a get_params and put_params on the pdfwrite device. Presumably this is the action of setpagedevice.

If ColorConversionStrategy is set to /RGB then we hit the code in gdevpdfp.c, gdev_pdf_put_param_impl():

        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;

This counts down the icc_struct which is currently being shared with the nulldevice, to 1. We then assign a new icc_struct to the pdfwrite device and initialise it. This sets icc_struct->device_profile[0], and that turns out to be the same ICC profile as in the shared icc_struct, the default RGB profile.

We then grestore the nulldevice away, this counts down icc_struct, to 0, which causes it to be released. That frees the memory pointed to by device_profile[0].

Unfortunately, this is still being pointed at by the pdfwrite device's icc_struct->device_profile[0].

Any attempt to then *use* that profile may cause a crash, if the memory has been reused or overwritten.

I'm not at all clear on why we dereference the icc_struct member, this seems to be the root of the problem. We should either set it to NULL after dereferencing it, or we should not dereference it, I'm not clear on which is the correct behaviour. Setting the pointer to NULL after dereferencing causes the file to run to completion without error.
Comment 1 Michael Vrhel 2020-11-02 22:26:16 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).
Comment 2 Ken Sharp 2020-11-03 08:20:35 UTC
(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.
Comment 3 Michael Vrhel 2020-11-03 19:32:50 UTC
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.
Comment 4 Ken Sharp 2020-11-04 09:11:29 UTC
Fixed in commit e6d34e7d08dac91b5b3b858c23e186a0d3bcbefc