Issue 120165 - [From Symphony]Impress crashed when play screen show with sample file
[From Symphony]Impress crashed when play screen show with sample file
Status: CLOSED FIXED
Product: Impress
Classification: Application
Component: editing
3.4.0
All All
: P3 critical (vote)
: 4.0.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 08:01 UTC by Yan Ji
Modified: 2012-11-07 08:36 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
sample (902.04 KB, application/octet-stream)
2012-07-03 08:01 UTC, Yan Ji
no flags Details
Fix patch for Bug 120165 (2.84 KB, patch)
2012-07-27 03:43 UTC, pengyunquan
no flags Details | Diff
Fix patch for Bug 120165 (2.87 KB, patch)
2012-07-27 04:37 UTC, pengyunquan
yunquanp: review?
Details | Diff
Fix by correcting Mask/Alpha at BitmapEx construction time (1.55 KB, patch)
2012-07-27 09:51 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Yan Ji 2012-07-03 08:01:28 UTC
Created attachment 78561 [details]
sample

Build: AOO 3.4

Open attached sample file, then play screen show

Defect: Application crashed
Comment 1 pengyunquan 2012-07-27 03:43:48 UTC
Created attachment 78747 [details]
Fix patch for Bug 120165

The sample file contains .svm files which contains bitmapex whose mask is not the same size as its bitmap. And this cause access out of boundary in GraphicManager::ImplCreateScaled
Comment 2 pengyunquan 2012-07-27 03:46:29 UTC
Comment on attachment 78747 [details]
Fix patch for Bug 120165

update a patch with full source file path later
Comment 3 pengyunquan 2012-07-27 04:37:00 UTC
Created attachment 78748 [details]
Fix patch for Bug 120165

Update fix patch
Comment 4 Andre 2012-07-27 08:00:28 UTC
@pengyunquan: I have two questions regarding the fix:
1. Instead of introducing two new variables nDstWV and nDstHV, would it be possible to modify and reuse the existing ones nDstW and nDstH?

2. The use pMapLX and pMapLY in lines 1251 and 1252 indicates that their values are sorted.  Is that true?
Comment 5 Armin Le Grand 2012-07-27 09:40:19 UTC
ALG: I have even more questions to this crash and fix.
- Why only fix for Mask, BitmapEx can also have Alpha with the same problem?
- Why not fix in a way that alpha/mask gets scaled and not just truncated?
- The BitmapEx constructor already asserts unequal sizes, this imples that unequal sizes are not wanted from design. Why not fix there?

I will propose an other xix/patch.
Comment 6 Armin Le Grand 2012-07-27 09:51:59 UTC
Created attachment 78754 [details]
Fix by correcting Mask/Alpha at BitmapEx construction time

ALG: Patch added which corrects given Alpha/Mask at BitmapEx construction time, thus ensuring unequal pixel sizes will not happen. This makes all code based on BitmapEx pixel magic involving Alpha/Mask more simple and safe.
Comment 7 Andre 2012-07-27 12:09:46 UTC
@ALG: Is this an additional patch or does it replace the previous one?

@pengyunquan: Is the second patch OK for you?
Comment 8 Armin Le Grand 2012-07-27 12:42:36 UTC
ALG: It replaces the original one. It fixes the general problem root by adapting the mask/alpha bitmap at BitmapEx construction time if not the same pixel size (there was an assert there already indicating that this is not wanted).
Comment 9 Andre 2012-07-27 14:39:53 UTC
Then go ahead and check in your changes.
Comment 10 SVN Robot 2012-07-27 15:04:44 UTC
"alg" committed SVN revision 1366405 into trunk:
#120165# Adapt Mask/Alpha at BitmapEx construction when size differs from bas...
Comment 11 Armin Le Grand 2012-07-27 15:05:10 UTC
ALG: Okay, comitted as revision 1366405, done.
Comment 12 pengyunquan 2012-07-31 06:15:46 UTC
I have one question: If we scale the mask bitmap, is the mask bitmap still a valid mask? As we know even a pixel offset of mask bitmap will get quite different mask result.
Comment 13 pengyunquan 2012-07-31 06:23:42 UTC
If we scale the mask bitmap, we changed the saved BitmapEx object. I know it is an abnormal BitmapEx object, and we don't know how this abnormal object is generated. But I think we'd better keep the abnormal BitmapEx object to avoid risk of incorrect display result.
Comment 14 Armin Le Grand 2012-07-31 07:27:57 UTC
ALG->pengyunquan: When the mask has not the same pixel size, it can anyways not fit the content; the mask will anyways been scaled when painting with VCL (not truncated as in the patch), so scaling it at BitmapEx construction time makes no difference for visualization, except that the visualisation does the scaling at runtime.
Changing the saved BitmapEx: Yes, it will be saved changed, but I think this is acceptable for such bitmaps. As explained above, VCL will scale the Mask/Alpha at output to the same output pixel size anyways. All pixel formats I know which support Alpha/Mask have something like RGBA anyways in file formats, thus a unequal SizePixel is not even possible/allowed since one alpha value is bound to one pixel.
I also do not know how that BitmapEx was created, but as the existing asserts show it is not intended and leads to many exceptions in the whole office. In many places are asserts to warn in this case (as in the task case there were already warnings), but no handling implemented. I have not designed VCL, but this looks like purpose, too.
If there should really be a case where this is problematic we can react on it when it comes up.
Comment 15 louqle 2012-08-20 07:18:56 UTC
verified on XP SP3 and Ubuntu 12.04 against trunk build 1374181
Comment 16 louqle 2012-08-23 00:52:10 UTC
Verified on xp, Ubuntu, win7,mac,win7-64,Vista,suse,redhat, SLED 11 SP1 64 bit, all pass, close this bug
Comment 17 Shenfeng Liu 2012-11-07 08:36:00 UTC
set Target Milestone to AOO 3.5.0 for PM purpose.