The test file location is given in the "URL" field, and must be downloaded
with ssh (account on casper required). It is 53 Mb.

This affects a P1 customer.

Created attachment 4169 [details]
fix_12983.pat

This should be reviewed by Igor, but I STRONGLY suggest adding cautionary
comments
in to help make this less likely in the future.

Created attachment 4170 [details]
patch.txt

The patch 4169 has portability problems, attaching an improved one. I can't
test the 53M file before Thirsday because I'm on an extremily slow and
expensive GPRS connection. I'll add a result of a local testing with
comparefiles when it is ready.

Local testring didn't detect any problem.

Yes, this portability is better (my patch was "quick and dirty" and assumed
4-byte floats). Also I had missed that the CTM was actually variable length up
to 25 bytes, not the 10 bytes I had observed in the problem.

Also, I like the improvement that uses this #define constant to allocate the
stack based 'buf' since clearly we previously had a buffer overflow potential
(by at least the size of the bbox).

It would be nicer to check the size BEFORE writing the buffer, however. The
current code only checks and returns an error after the damage is done.

One question:

Why is the 'mask_id' included twice in the MAX_CLIST_COMPOSITOR_SIZE calculation?

! 	     sizeof(((gs_pdf14trans_params_t *)0)->mask_id) + \
! 	     sizeof(((gs_pdf14trans_params_t *)0)->Background) + \
! 	     sizeof(((gs_pdf14trans_params_t *)0)->GrayBackground) + \
! 	     sizeof(((gs_pdf14trans_params_t *)0)->transfer_fn) + \
! 	     sizeof(((gs_pdf14trans_params_t *)0)->mask_id) )

I will test the patch against the 53Mb file (after removing the duplicate
mask_id).

The 53Mb file runs fine with the patch (modified to remove the duplicate
mask_id in the size calculation).

Note I still recommend adding a comment to the code, but perhaps since the
'buf' now refers to the #define constant, the linkage is a bit more obvious.

A change to detect potential overflow BEFORE writing the buffer is warranted,
IMHO, otherwise, why have a check at all? After all, buffer overflows of stack
based buffers are frequent exploits for security breaches.

Marcos,

You wrote : "It would be nicer to check the size BEFORE writing the buffer". 
Hmm, it may be done with some intrivial change: (1) create a stream with 
swrite_position_only, (2) write all data to there, (3) check the actuall 
length, (4) create another stream with swrite_string, (5) write to it. It will 
require to convert the serialization code into one with a stream output. I 
don't think it worth the trouble : the simpler post-factum check worked fine 
(since the bug is open), and there are many other important things to do. We 
already have got a similar logic in cmd_put_drawing_color, but its maintanance 
appears too hard. Actually what I coded is a simplified *overestimation* of 
the required length.

You wrote : "Note I still recommend adding a comment to the code". I added : 
buf /* Must be large enough to fit the data written below. */ and 
/*
   The longest compositor command includes transparency mask parameters
   (see gdevp14.c c_pdf14trans_write() the "case PDF14_BEGIN_TRANS_MASK"). */
Isn't it sufficient ?

You wrote : "Why is the 'mask_id' included twice". Oops, the second one has to 
be removed.

Marcos, do you want to comit the patch and close the bug, or you want me to do 
that ? I think it is mainly your product.    				
		      

Igor,

The comments (and the original patch) were from me (Ray) not Marcos.

Please commit the patch (you are the owner of the clist logic), unless you
feel that this is a transparency issue in which case I will consult with Michael
(he is the owner of the color and transparency code).

Since you reviewed and improved this patch, I think you should commit it.

On the topic of detecting overflow BEFORE writing, I think it is safe to assume
that there will always be enough space for the "fixed length" elements, and
simply do checking before writing the larger chunks (Background Color * number
of BG components) and transfer function. The check can be as simple as
   if ((pbuf+l+sizeof(GrayBackground)-buf) >
        MAX_CLIST_TRANSPARENCY_COMPOSITOR_SIZE)

Also, it appears from another examination of the code, that the
'transfer_fucntion' is never written to the 'buf' array, but rather is copied
directly from pparams->transfer_function, so the:

   sizeof(((gs_pdf14trans_params_t *)0)->transfer_fn) 

does not need to be included in the buf size, but it _IS_ part of the entire
compositor size. So it seems that you could reduce the buffer allocation to
       byte buf[MAX_CLIST_TRANSPARENCY_COMPOSITOR_SIZE -
                sizeof(((gs_pdf14trans_params_t *)0)->transfer_fn)];

I'm rather just as happy to have the wasted 256 bytes on the stack as extra
guarantee against buffer overflow.

Please use your own judgement.

Regarding to Comment #8 : 

Well I'm happy that collegues pay so big attention to this simple patch. 
Spending a lot of man*hours we will come to a really optimal solution. 
Ray's "simple" check doesn't account the number of components of Background 
(not sure why), so it neeads further improvements. transfer_fn is not written 
to the buffer but must be accounted in MAX_CLIST_TRANSPARENCY_COMPOSITOR_SIZE, 
which is used elsewhere. Well we can save 256 butes from the single element of 
the stack with a further complexication of #defines.

Nevertheless the great problem here is the number of assumptions done when 
coding the check. Ray suggests to add one more : there exist fixed size 
elements, which may be estimated in advance. One can prove it with reading 
put_value, etc. Well it has to be documented. But IMO going in this way we 
fall into a risk of making logical errors while combining the assumptions. 
Thus the save solution is as explained in Comment 7, but it is too expewnsive 
in both man*hours and in CPU time, everything else is partially safe and 
partially risky. I'm not sure that we're doing a right thing spending so many 
man*hours for seeking for the optimal compromise between two vague issues, 
which both are not well defined. 

Due to all that I suggesteg to continue this effort to those who likes it. 
Otherwise I'll commit it tomorrow. My local time is too late, and it gives a 
gap for others.

Now I slept well and reviewed it with a fresh head. Neither old nor new code 
implements a check for the local buffer overflow. The check is about 
overflowing the clist interpreter buffer, which is unrelated to the local 
buffer. The stack damage appears with the old code because the local buffer 
was too small : it's length was not updated when the writing code was enhanced 
with bbox in revision 8587. 

So now I discontinue the effort to implement a dynamic check for the local 
buffer overflow, and provide better comments. The patch is being tested 
locally (except for 58M file) and will be committed in few hours.

Also I saved 256K of stack from transfer_fn. Nevertheless the expense for 
Background still persists. This field consumes 8 times bigger. 

If Support wants further improvements, a separate bug (or enhancement ?) to be 
opened and assigned to the module owner. The module was initially written by 
Raph and later it was fragmentary rewritten by Leo, so I believe one can find 
many other inaccurate things in it. As to me. I'll return to the P1 bug I'm 
working now.

Patch to HEAD :
http://ghostscript.com/pipermail/gs-cvs/2008-July/008396.html