Bug 695050 - wrong code in combine escapes across non-small block of input.
Summary: wrong code in combine escapes across non-small block of input.
Status: RESOLVED FIXED
Alias: None
Product: GhostPCL
Classification: Unclassified
Component: PCL interpreter (show other bugs)
Version: unspecified
Hardware: PC Linux
: P4 normal
Assignee: Henry Stiles
URL:
Keywords:
Depends on:
Blocks: 695047
  Show dependency tree
 
Reported: 2014-02-17 14:48 UTC by Hin-Tak Leung
Modified: 2014-03-12 16:35 UTC (History)
0 users

See Also:
Customer:
Word Size: ---


Attachments
patch, to make it possible to combine escapes across planes of raster. (2.84 KB, patch)
2014-02-17 14:48 UTC, Hin-Tak Leung
Details | Diff
better patch (2.91 KB, patch)
2014-02-17 20:05 UTC, Hin-Tak Leung
Details | Diff
PCL short hand improvement (2.55 KB, patch)
2014-03-11 08:00 UTC, Henry Stiles
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.