Bug 695050

Summary:	wrong code in combine escapes across non-small block of input.
Product:	GhostPCL	Reporter:	Hin-Tak Leung <htl10>
Component:	PCL interpreter	Assignee:	Henry Stiles <henry.stiles>
Status:	RESOLVED FIXED
Severity:	normal
Priority:	P4
Version:	unspecified
Hardware:	PC
OS:	Linux
Customer:		Word Size:	---
Bug Depends on:
Bug Blocks:	695047
Attachments:	patch, to make it possible to combine escapes across planes of raster. better patch PCL short hand improvement

Description Hin-Tak Leung 2014-02-17 14:48:04 UTC

Created attachment 10707 [details]
patch, to make it possible to combine escapes across planes of raster.

Currently the pcl parser code only works correctly for combined escapes for small arguments (by repeatedly dropping through scanning_parameters). But if the parameter size exceeds the parser buffer, and needs to go to scanning_data, it does not ever return to continue.

This is observed with attachment 10700 [details] and attachment 10701 [details] (which have CMY and KCMY mode 9 raster, respectively). The driver writes 

<ESC>*b<count1>v<plane1data><count2>v<plane2data><count3>w<plane3data> ...

somewhat continuously, with very few <ESC>*b to separate escape sequences. This is valid PCL (just poorly constructed for redundancy and random access) but mis-processed.

Attached is a patch which corrects the flaw. The approach is probably considered a bit ugly for adding a single-use state variable though.

Comment 1 Hin-Tak Leung 2014-02-17 14:49:48 UTC

Comment on attachment 10707 [details]
patch, to make it possible to combine escapes across planes of raster.

correct mime type.

Comment 2 Hin-Tak Leung 2014-02-17 15:47:50 UTC

The patch causes about 1/4 of the pcl files in cluster tests to behave differently, so it is not quite correct. One such file is tools/owl.pcl .

Comment 3 Hin-Tak Leung 2014-02-17 20:05:33 UTC

Created attachment 10708 [details]
better patch

better patch. cluster tested & no difference.

Comment 4 Henry Stiles 2014-03-11 08:00:45 UTC

Created attachment 10744 [details]
PCL short hand improvement

Thank for looking at this Hin-Tak, certainly worth a "bounty".  I think this patch is a little better though, simpler at least, have a review, I don't know if it addresses all the files you've encountered, it does parse tiger.pjl.

Comment 5 Hin-Tak Leung 2014-03-11 11:16:08 UTC

(In reply to comment #4)
> Created attachment 10744 [details]
> PCL short hand improvement
> 
> Thank for looking at this Hin-Tak, certainly worth a "bounty".  I think this
> patch is a little better though, simpler at least, have a review, I don't
> know if it addresses all the files you've encountered, it does parse
> tiger.pjl.

I think it is mostly algorithmic equivalent, except for two things - the gs_free_object() on data on heap, and the 'param_init(); continue;' vs break part. 'param_init(); continue;' goes back to 'while (p < rlimit)', while 'break'
means processes the part after switch() {}. I don't think I understand that code well enough, but I think during the earlier testing I experienced some problems dropping down to the part after switch()?

You can always put a 'bountiable' in :-).

Comment 6 Henry Stiles 2014-03-11 16:14:06 UTC

Go ahead and claim a bounty on this.  I review all the "bounties" and I'll okay it.

Comment 7 Hin-Tak Leung 2014-03-12 16:35:01 UTC

The patch was committed:

commit ee30f349671e3a23d666615f1648eab4a5290b4c
Author: Henry Stiles <henry.stiles@artifex.com>
Date:   Tue Mar 11 09:58:31 2014 -0600

    Abbreviated escape sequence improvement.
    
    Abbreviated escape sequences were not handled properly with commands
    that hold arbitrary amounts of data, like raster and font related
    commands.  PCL shorthand is rarely used in this circumstance
    but it is legal.  Thanks to Hin-Tak for the discovery and analysis of
    this problem.

I assumed Henry meant to close this? It is rare enough that the memory leak is probably not important, and if I see any issue with break vs continue I'd either reopen or file new.