Re: Bug 687560 "Invalid PDF if /BP pdfmarkswithnon-unique /_objdef"

Dear SaGS,

> From: "SaGS" <[email protected]>
> To: "gs-code-review" <[email protected]>
> Cc: "Igor V. Melichev" <[email protected]>
> Sent: Monday, November 15, 2004 12:31 AM
> Subject: Re: Bug 687560 "Invalid PDF if /BP pdfmarkswithnon-unique
> /_objdef"


> Dear Igor,
>
> I already did a few changes to the patch, but before posting a new version
> we need to agree on some of the points. So, here are some explanations and
> questions.
>
> ----- Original Message -----
> From: "Igor Melichev" <[email protected]>
> To: "SaGS" <[email protected]>; <[email protected]>
> Sent: Thursday, 4 November 2004 20:01
> Subject: Re: [gs-code-review] Fix: Bug 687560 "Invalid PDF if /BP
> pdfmarkswithnon-unique /_objdef"
>
>
>> ...
>> First of all, please follow C-style.htm about the code formatting.
>> ...
>
> OK. Also got "indent", a 98% friend.
>
>
>> ...
>> 1. The function cos_dict_rename looks some unsmooth
>> and it's name isn't enough leading. Could you please
>> replace its usage with 2 "traditional" operations :
>> remove_entry(old_name),  add_entry(new_name) ?
>> (Yes, "rename" saves a few CPU time with
>> memory allocations, but it doesn't looks so important optimization).
>> Or at least cos_dict_rename to be renamed with
>> cos_dict_rename_entry.
>
> (1.A) The name "cos_dict_rename_entry" is fine with me. I considered
> cos_dict_rename[_entry]() can be reused when (if ever) other pdfmarks will
> be fixed to deal with duplicate distill-time names. (Haven't looked
> closely,
> but it looks this is somehow random at present: /ANN raises an error, /OBJ
> implementation is entirely "original" and /NI creates an invalid PDF the
> same way as /BP and /PS do at present.)
>
> (1.B) There are already plenty of "put()". If you think that a "remove()"
> will be much more useful, then that's fine with me too and I'll change the
> code. I suggest the name "cos_dict_unadd()", as its array counterpart. The
> function will remove an entry from the dictionary, free the key if
> necessary, and return the value (the value will NOT be freed).
>
> Please decide between (1.A) and (1.B).

Well, lets go with "remove". I don't like "unadd"
for this function. A parallel with arrays isn't smooth.

>
>
>> 2. I'm unclear with the implementation of the branch
>> PDF_NAMECOLLISION_REBUILD.
>> Do you really need the "surgery" ?
>> I would recommend to do it as this :
>> 2.1. Create a new object with pdf_enter_substream;
>> 2.2. Copy all useful keys from the old cos dict to the new one;
>> 2.3. Remove the old object from the named object list,
>> and add the new one;
>> 2.4. Cancel and free the old object.
>> Note that cos_write_stream_redir, cos_stream_move_all become unnecessary.
>> (Well, my suggestion won't work if there are references to
>> the old object from others. But in this case your code
>> doesn't work also).
>
> Let not say "reference", I consider this word too imprecise. It can mean:
> (a) A C pointer. I'll name these "C-ptrs";
> (b) References based exclusively on object numbers, those "n 0 R" or
> "/Rn".
> I'll name these "n 0 Refs".
> (c) The construct "{OBJNAME}" when it appears in the value part of a
> key-value pair and the key is anything except /_objdef. I'll name these
> "{OBJRefs}". (These appear only in the source PostScript code, and are
> converted internally to "n 0 Refs", but that's not an important fact
> here.)
>
> With the latest version of the patch, "C-ptrs" remain valid because
> neither
> the old cos_object_t nor the old pdf_resource_t are freed or reallocated,
> only their contents may change. Also, the object number and name remain,
> so
> "n 0 Refs" and "{OBJRefs}" are OK too. What exactly makes you believe "in
> this case [the] code doesn't work also"?

Well, I withdraw the last sentence.

> Notes:
> - With the preceding version of the patch, "C-ptrs" didn't survive a
> PDF_NAMECOLLISION_REBUILD;
> - While I didn't find a "C-ptr" to an XObject, I'm not confident that such
> pointers don't exist, nor that they won't be used in the future; for this
> reason, I prefer not to change the patch to conform to 2.1 ... 2.4 above.
>
> Regarding the "surgery", I don't like the idea either, because it's a
> source
> of errors. In fact, in the meantime I discovered that, even if the patient
> did not die, the surgery was incomplete,
> pdev->accumulating_substream_resource needing to be patched too.
>
> Given this situation, I think the options are:
> (2.A) Change pdf_alloc_aside()

I don't like to chage basic functions,
which were created a long ago by old programmers.
I'm uneasy to foresee consequences, so we'll better keep them
as they are. Note that I keep in mind multiple projects,
and therefore I'm interested to simplify any of tham and to make
their code permanent, at least in the functtional basis.

> as it was in the preceding version of the
> patch. It will continue to output an "aside", but the added "pcofwd"
> parameter allows the caller, by setting this to !=NULL, to feed-in
> whatever
> pieces of the future "aside" are already available. This way, the right
> object is used in the first place, all those intricate connections will be
> made to the right object, and "surgery" will be unnecessary.
> (pdf_enter_substream() and pdf_open-aside() will also need this extra
> parameter, but their logic doesn't really change, they only pass this
> parameter to the next function.) A VARIATION of this: don't add an extra
> parameter, but make the existing "ppres" to be an in/out parameter rather
> than an out parameter. (In this 2nd case, I identified a few places in 4
> files - including gdevpdfm.c - where *ppres needs to be initialized to 0.)
>
> (2.B) Let the "surgery" as-is (with the rectification about
> pdev->accumulating_substream_resource I mentioned).
>
> Please take a decision on this.

Hmm, I have no time to dig into this now.
If I remove "2.4. Cancel and free the old object"
from my suggestion above, could it work ?


>
>
>>
>> 3. Please note that recently I defined pdf_forget_resource (see
>> http://www.ghostscript.com/pipermail/gs-cvs/2004-November/004992.html),
>> which may be useful for your code.
>> ...
>
> OK, it seems I can simply replace pdf_cancel_resource() with this one
> (except it returns void, not a "code").
>
>
>> 4. It would be useful to group (factor out)
>> the call to pdf_enter_substream
>> and pdev->substream_Resources = cos_dict_alloc(...)
>> into a special local function pdf_enter_BP
>> (Well, I'm not in favor with the name pdf_enter_BP,
>> fell free to improve it). The purposes are :
>> 4.1. do not distribute redundant specifics of BP through GS code;
>> 4.2. Simplify the view of the big operator "switch" you introduced;
>
> OK. For the name, I'll choose "pdf_enter_XObject" (being used not only for
> /BP, but for /PS too), unless you have another preference.
>
>> 4.3. Move /* Have no better switch*/ to a single place.
>
> While writing the new pdf_enter_XObject(), I noticed an unused parameter
> of
> start_XObject(). It looks we do have a better choice: the "compress"
> option
> passed from the caller. I'll use this, unless you find a reason not to. It
> is set by the caller to pdev->CompressPages, not pdev->CompressFonts,
> which
> looks more appropriate; and anyway it's a good idea to let the caller
> decide
> on compression. If this change is not OK with you, it's the moment to say
> so.

This change looks good, but I would not mix it within same patch.
Lets delay it for a while.

>
>
>> 5. You coded :
>>
>> +           if (!(oper &PDF_NAMECOLLISION_NONAME))
>> +               code = cos_dict_put(pdev->local_named_objects,
>> objname->data,
>> +                                   objname->size,
> cos_object_value(&value,
>> pres->object));
>> +           else
>> +               DO_NOTHING;
>> +           if (code < 0)
>> +               return code;
>> ...
>
> Stupid mistake, sorry.
>
>
> Sincerely yours,
> SaGS
>

So we more or less in a complaint except 2B.
I'm sorry that I had no more time for analyzing it.
Your contribution would be useful.

Igor.