From 23499be58677663ba69d2b29618bf05c4366aa34 Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Thu, 23 Aug 2018 12:20:56 +0100 Subject: [PATCH 01/13] Bug 699668: handle stack overflow during error handling When handling a Postscript error, we push the object throwing the error onto the operand stack for the error handling procedure to access - we were not checking the available stack before doing so, thus causing a crash. Basically, if we get a stack overflow when already handling an error, we're out of options, return to the caller with a fatal error. --- psi/interp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/psi/interp.c b/psi/interp.c index 8b49556..6150838 100644 --- a/psi/interp.c +++ b/psi/interp.c @@ -676,7 +676,12 @@ again: /* Push the error object on the operand stack if appropriate. */ if (!GS_ERROR_IS_INTERRUPT(code)) { /* Replace the error object if within an oparray or .errorexec. */ - *++osp = *perror_object; + osp++; + if (osp >= ostop) { + *pexit_code = gs_error_Fatal; + return_error(gs_error_Fatal); + } + *osp = *perror_object; errorexec_find(i_ctx_p, osp); } goto again; -- 2.14.4 From 3f5427b4ee15054127a9127bfbc6e1b1371992ca Mon Sep 17 00:00:00 2001 From: Ken Sharp Date: Thu, 23 Aug 2018 15:42:02 +0100 Subject: [PATCH 02/13] Bug 699665 "memory corruption in aesdecode" The specimen file calls aesdecode without specifying the key to be used, though it does manage to do enough work with the PDF interpreter routines to get access to aesdecode (which isn't normally available). This causes us to read uninitialised memory, which can (and often does) lead to a segmentation fault. In this commit we set the key to NULL explicitly during intialisation and then check it before we read it. If its NULL we just return. It seems bizarre that we don't return error codes, we should probably look into that at some point, but this prevents the code trying to read uninitialised memory. --- base/aes.c | 3 +++ base/saes.c | 1 + 2 files changed, 4 insertions(+) diff --git a/base/aes.c b/base/aes.c index a6bce93..e86f000 100644 --- a/base/aes.c +++ b/base/aes.c @@ -662,6 +662,9 @@ void aes_crypt_ecb( aes_context *ctx, } #endif + if (ctx == NULL || ctx->rk == NULL) + return; + RK = ctx->rk; GET_ULONG_LE( X0, input, 0 ); X0 ^= *RK++; diff --git a/base/saes.c b/base/saes.c index 6db0e8b..307ed74 100644 --- a/base/saes.c +++ b/base/saes.c @@ -120,6 +120,7 @@ s_aes_process(stream_state * ss, stream_cursor_read * pr, gs_throw(gs_error_VMerror, "could not allocate aes context"); return ERRC; } + memset(state->ctx, 0x00, sizeof(aes_context)); if (state->keylength < 1 || state->keylength > SAES_MAX_KEYLENGTH) { gs_throw1(gs_error_rangecheck, "invalid aes key length (%d bytes)", state->keylength); -- 2.14.4 From b47d8dc0bdea68a2b05ece2b1ee05becb9667ee9 Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Thu, 23 Aug 2018 15:41:18 +0100 Subject: [PATCH 03/13] Bug 699664: Ensure the correct is in place before cleanup If the PS job replaces the device and leaves that graphics state in place, we wouldn't cleanup the default device in the normal way, but rely on the garbage collector. This works (but isn't ideal), *except* when the job replaces the device with the null device (using the nulldevice operator) - this means that .uninstallpagedevice doesn't replace the existing device with the nulldevice (since it is already installed), the device from the graphics ends up being freed - and as it is the nulldevice, which we rely on, memory corruption and a segfault can happen. We avoid this by checking if the current device is the nulldevice, and if so, restoring it away, before continuing with the device cleanup. --- psi/imain.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/psi/imain.c b/psi/imain.c index 2fe1546..138bfc8 100644 --- a/psi/imain.c +++ b/psi/imain.c @@ -936,6 +936,16 @@ gs_main_finit(gs_main_instance * minst, int exit_status, int code) i_ctx_p = minst->i_ctx_p; /* interp_reclaim could change it. */ } + if (i_ctx_p->pgs != NULL && i_ctx_p->pgs->device != NULL && + gx_device_is_null(i_ctx_p->pgs->device)) { + /* if the job replaced the device with the nulldevice, we we need to grestore + away that device, so the block below can properly dispense + with the default device. + */ + int code = gs_grestoreall(i_ctx_p->pgs); + if (code < 0) return_error(gs_error_Fatal); + } + if (i_ctx_p->pgs != NULL && i_ctx_p->pgs->device != NULL) { gx_device *pdev = i_ctx_p->pgs->device; const char * dname = pdev->dname; -- 2.14.4 From cd1caf1f63f645260481a73d9990ec28cf9ba79b Mon Sep 17 00:00:00 2001 From: Chris Liddell Date: Thu, 23 Aug 2018 14:13:25 +0100 Subject: [PATCH 04/13] Bug 699661: Avoid sharing pointers between pdf14 compositors If a copdevice is triggered when the pdf14 compositor is the device, we make a copy of the device, then throw an error because, by default we're only allowed to copy the device prototype - then freeing it calls the finalize, which frees several pointers shared with the parent. Make a pdf14 specific finish_copydevice() which NULLs the relevant pointers, before, possibly, throwing the same error as the default method. This also highlighted a problem with reopening the X11 devices, where a custom error handler could be replaced with itself, meaning it also called itself, and infifite recursion resulted. Keep a note of if the handler replacement has been done, and don't do it a second time. --- base/gdevp14.c | 17 ++++++++++++++++- devices/gdevxini.c | 12 ++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/base/gdevp14.c b/base/gdevp14.c index d972fc2..7cf81b2 100644 --- a/base/gdevp14.c +++ b/base/gdevp14.c @@ -178,6 +178,7 @@ static dev_proc_fill_mask(pdf14_fill_mask); static dev_proc_stroke_path(pdf14_stroke_path); static dev_proc_begin_typed_image(pdf14_begin_typed_image); static dev_proc_text_begin(pdf14_text_begin); +static dev_proc_finish_copydevice(pdf14_finish_copydevice); static dev_proc_create_compositor(pdf14_create_compositor); static dev_proc_create_compositor(pdf14_forward_create_compositor); static dev_proc_begin_transparency_group(pdf14_begin_transparency_group); @@ -245,7 +246,7 @@ static const gx_color_map_procs * pdf14_create_compositor, /* create_compositor */\ NULL, /* get_hardware_params */\ pdf14_text_begin, /* text_begin */\ - NULL, /* finish_copydevice */\ + pdf14_finish_copydevice, /* finish_copydevice */\ pdf14_begin_transparency_group,\ pdf14_end_transparency_group,\ pdf14_begin_transparency_mask,\ @@ -3952,6 +3953,19 @@ pdf14_text_begin(gx_device * dev, gs_gstate * pgs, return code; } +static int +pdf14_finish_copydevice(gx_device *new_dev, const gx_device *from_dev) +{ + pdf14_device *pdev = (pdf14_device*)new_dev; + + pdev->ctx = NULL; + pdev->trans_group_parent_cmap_procs = NULL; + pdev->smaskcolor = NULL; + + /* Only allow copying the prototype. */ + return (from_dev->memory ? gs_note_error(gs_error_rangecheck) : 0); +} + /* * Implement copy_mono by filling lots of small rectangles. */ @@ -8113,6 +8127,7 @@ c_pdf14trans_clist_read_update(gs_composite_t * pcte, gx_device * cdev, before reopening the device */ if (p14dev->ctx != NULL) { pdf14_ctx_free(p14dev->ctx); + p14dev->ctx = NULL; } dev_proc(tdev, open_device) (tdev); } diff --git a/devices/gdevxini.c b/devices/gdevxini.c index 8511eac..23b8c35 100644 --- a/devices/gdevxini.c +++ b/devices/gdevxini.c @@ -59,7 +59,8 @@ static struct xv_ { Boolean alloc_error; XErrorHandler orighandler; XErrorHandler oldhandler; -} x_error_handler; + Boolean set; +} x_error_handler = {0}; static int x_catch_alloc(Display * dpy, XErrorEvent * err) @@ -74,7 +75,8 @@ x_catch_alloc(Display * dpy, XErrorEvent * err) int x_catch_free_colors(Display * dpy, XErrorEvent * err) { - if (err->request_code == X_FreeColors) + if (err->request_code == X_FreeColors || + x_error_handler.orighandler == x_catch_free_colors) return 0; return x_error_handler.orighandler(dpy, err); } @@ -274,8 +276,10 @@ gdev_x_open(gx_device_X * xdev) return_error(gs_error_ioerror); } /* Buggy X servers may cause a Bad Access on XFreeColors. */ - x_error_handler.orighandler = XSetErrorHandler(x_catch_free_colors); - + if (!x_error_handler.set) { + x_error_handler.orighandler = XSetErrorHandler(x_catch_free_colors); + x_error_handler.set = True; + } /* Get X Resources. Use the toolkit for this. */ XtToolkitInitialize(); app_con = XtCreateApplicationContext(); -- 2.14.4 From a6b4cbf5def8a61013f2ef787d6618328cdf92ef Mon Sep 17 00:00:00 2001 From: Ken Sharp Date: Thu, 23 Aug 2018 14:12:48 +0100 Subject: [PATCH 05/13] Fix Bug 699660 "shading_param incomplete type checking" Its possible to pass a t_struct parameter to .shfill which is not a shading function built by .buildshading. This could then lead to memory corruption or a segmentation fault by treating the object passed in as if it were a shading. Its non-trivial to check the t_struct, because this function can take 7 different kinds of structures as a parameter. Checking these is possible, of course, but would add a performance penalty. However, we can note that we never call .shfill without first calling .buildshading, and we never call .buildshading without immediately calling .shfill. So we can treat these as an atomic operation. The .buildshading function takes all its parameters as PostScript objects and validates them, so that should be safe. This allows us to 'hide' the .shfill operator preventing the possibility of passing an invalid parameter. --- Resource/Init/gs_init.ps | 4 ++-- Resource/Init/gs_ll3.ps | 7 ++++++- Resource/Init/pdf_draw.ps | 3 +-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Resource/Init/gs_init.ps b/Resource/Init/gs_init.ps index 6c8da53..1956ed5 100644 --- a/Resource/Init/gs_init.ps +++ b/Resource/Init/gs_init.ps @@ -2181,8 +2181,8 @@ SAFER { .setsafeglobal } if /.getiodevice /.getdevparms /.putdevparams /.bbox_transform /.matchmedia /.matchpagesize /.defaultpapersize /.oserrno /.setoserrno /.oserrorstring /.getCPSImode /.getscanconverter /.setscanconverter /.type1encrypt /.type1decrypt/.languagelevel /.setlanguagelevel /.eqproc /.fillpage /.buildpattern1 /.saslprep -/.buildshading1 /.buildshadin2 /.buildshading3 /.buildshading4 /.buildshading5 /.buildshading6 /.buildshading7 /.buildshadingpattern -/.argindex /.bytestring /.namestring /.stringbreak /.stringmatch /.globalvmarray /.globalvmdict /.globalvmpackedarray /.globalvmstring +/.buildshading1 /.buildshading2 /.buildshading3 /.buildshading4 /.buildshading5 /.buildshading6 /.buildshading7 /.buildshadingpattern +%/.shfill /.argindex /.bytestring /.namestring /.stringbreak /.stringmatch /.globalvmarray /.globalvmdict /.globalvmpackedarray /.globalvmstring /.localvmarray /.localvmdict /.localvmpackedarray /.localvmstring /.systemvmarray /.systemvmdict /.systemvmpackedarray /.systemvmstring /.systemvmfile /.systemvmlibfile /.systemvmSFD /.settrapparams /.currentsystemparams /.currentuserparams /.getsystemparam /.getuserparam /.setsystemparams /.setuserparams /.checkpassword /.locale_to_utf8 /.currentglobal /.gcheck /.imagepath diff --git a/Resource/Init/gs_ll3.ps b/Resource/Init/gs_ll3.ps index 5aa56a3..1d37e53 100644 --- a/Resource/Init/gs_ll3.ps +++ b/Resource/Init/gs_ll3.ps @@ -440,6 +440,11 @@ systemdict /.reuseparamdict mark /shfill .systemvar /undefined signalerror } ifelse } bind def + +/.buildshading_and_shfill { + .buildshading .shfill +} bind def + systemdict /.reuseparamdict undef /.buildpattern2 { %