Issue 121282 - nRowSpan has type sal_Int32, not long as inconsistently declared in lcl_TCFlags()
nRowSpan has type sal_Int32, not long as inconsistently declared in lcl_TCFla...
Status: CLOSED FIXED
Product: Writer
Classification: Application
Component: code
4.0.0-dev
All All
: P3 normal (vote)
: 4.0.0
Assigned To: Pedro Giffuni
:
Depends on:
Blocks: 121425
  Show dependency treegraph
 
Reported: 2012-10-27 02:06 UTC by Pedro Giffuni
Modified: 2013-07-12 11:09 UTC (History)
1 user (show)

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


Attachments
Test case (from LO) (25.00 KB, application/msword)
2012-10-29 14:36 UTC, Pedro Giffuni
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description Pedro Giffuni 2012-10-27 02:06:04 UTC
Using OpenGrok to check the definition of nRowSpan we find it defined in sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.

However, in file main/sw/source/filter/ww8/wrtww8.cxx, for the header of lcl_TCFlags() around line 1920 we see it is declared as a long.

This is likely to have negative effects, especially on 64 bit platforms.
Comment 1 Pedro Giffuni 2012-10-27 02:13:17 UTC
This bug seems to cause problems with merged cells.  The issue was initially detected in LibreOffice and their test case is reproduceable in AOO.

The LO fix was done by Caolan McNamara.
Comment 2 Oliver-Rainer Wittmann 2012-10-29 08:06:39 UTC
I will have a look at this issue.
Comment 3 Oliver-Rainer Wittmann 2012-10-29 08:55:13 UTC
(In reply to comment #0)
> Using OpenGrok to check the definition of nRowSpan we find it defined in
> sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> 

I did not find a <nRowSpan> defined as sal_Int32 in sw/source/filter/inc/wrtswtbl.hxx
Pedro: Can you please clarify?

> However, in file main/sw/source/filter/ww8/wrtww8.cxx, for the header of
> lcl_TCFlags() around line 1920 we see it is declared as a long.
> 
> This is likely to have negative effects, especially on 64 bit platforms.

Pedro: What would be the problem when assigning an sal_Int32 to an long? Especially on 64 bit platforms?
Comment 4 Oliver-Rainer Wittmann 2012-10-29 08:56:47 UTC
(In reply to comment #1)
> This bug seems to cause problems with merged cells.  The issue was initially
> detected in LibreOffice and their test case is reproduceable in AOO.
> 

Pedro: Can you attach at least one test case to this issue? Thx in advance.

> The LO fix was done by Caolan McNamara.
Comment 5 Pedro Giffuni 2012-10-29 14:33:20 UTC
(In reply to comment #3)
> (In reply to comment #0)
> > Using OpenGrok to check the definition of nRowSpan we find it defined in
> > sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> > 
> 
> I did not find a <nRowSpan> defined as sal_Int32 in
> sw/source/filter/inc/wrtswtbl.hxx
> Pedro: Can you please clarify?
> 

Looking in OpenGrok there are this lines:

http://opengrok.adfinis-sygroup.org/source/search?q=&defs=nRowSpan&refs=&path=&hist=&project=aoo-trunk

65 sal_uInt16 nRowSpan;	 // ueberspannte Zeilen member in class:SwWriteTableCell 
137 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
288 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,

but it appears defined as sal_uInt16 in other places as well.

> > However, in file main/sw/source/filter/ww8/wrtww8.cxx, for the header of
> > lcl_TCFlags() around line 1920 we see it is declared as a long.
> > 
> > This is likely to have negative effects, especially on 64 bit platforms.
> 
> Pedro: What would be the problem when assigning an sal_Int32 to an long?
> Especially on 64 bit platforms?

Problem is the size: there are no guarantees that the sizes match, which can cause either that data is lost or that overflows occur and we wont catch them. 

The fix is pretty easy: just have the sizes match by replacing the long in line 1920 with sal_uInt32. It's a local variable so it won't have any negative effect.
Comment 6 Pedro Giffuni 2012-10-29 14:36:23 UTC
Created attachment 79838 [details]
Test case (from LO)

Here is the LO test case:
-It is a file with a merged cell saved with MS Office.
-Open it with AOO (should look fine) and save it with another name without modifying anything.
-Close and reopen: the merged cell will appear split in two.
Comment 7 Pedro Giffuni 2012-10-30 04:16:30 UTC
(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > Using OpenGrok to check the definition of nRowSpan we find it defined in
> > > sw/source/filter/inc/wrtswtbl.hxx, and other places as sal_Int32.
> > > 
> > 
> > I did not find a <nRowSpan> defined as sal_Int32 in
> > sw/source/filter/inc/wrtswtbl.hxx
> > Pedro: Can you please clarify?
> > 
> 
> Looking in OpenGrok there are this lines:
> 
> http://opengrok.adfinis-sygroup.org/source/
> search?q=&defs=nRowSpan&refs=&path=&hist=&project=aoo-trunk
> 
> 65 sal_uInt16 nRowSpan;	 // ueberspannte Zeilen member in
> class:SwWriteTableCell 
> 137 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
> 288 sal_uInt16 nRowSpan, sal_uInt16 nColSpan,
> 
> but it appears defined as sal_uInt16 in other places as well.
> 

Ouch.. nRowSpan is defined as sal_uInt16 on some places and
sal_Int32 on other places, and also long on a few other places
but apparently only when it is a local value.

Both sal_uInt16 and sal_Int32 types should have the same size,
but there is no guarantee for long. Does it make sense to have
a negative nRowSpan ?
Comment 8 Oliver-Rainer Wittmann 2012-11-14 13:56:32 UTC
Yes, changing the parameter type of <nRowSpan> from long to sal_Int32 in method <lcl_TCFlags(...)> makes sense.
Comment 9 Oliver-Rainer Wittmann 2012-11-14 14:29:01 UTC
@Pedro: Can you please verify the fix? Thx in advance.
Additional to the intrinsic type change the parameter is now also constant and an If-statement which checks, if the value of the parameter is negative, is removed.
Comment 10 Pedro Giffuni 2012-11-26 21:47:55 UTC
I think the patch is correct but we still fail the testcase :(
Comment 11 Oliver-Rainer Wittmann 2012-12-03 08:44:57 UTC
@Pedro:
You are right.
The test case also fails on my system (Windows 7).
It looks like that I have overseen that <nRowSpan> values can become negative. The corresponding stl vector is defined as ::std::vector<sal_uInt32>. I thought that no negative values for <nRowSpan> could occur and removed the corresponding use case.

I will have a deeper look into the code.
Comment 12 Oliver-Rainer Wittmann 2012-12-03 11:49:08 UTC
My deeper look at the code and a further debugging session reveals that negative values for parameter <nRowSpan> of function <lcl_TCFlags(..)> are intended.

My former patch "went too far":
I changed the parameter type from <long> to <sal_uInt32>, because the parameter was filled with the value of an stl vector whose definition is ::std::vector<sal_uInt32>. But, actually negative values are stored inside this stl vector.

An corresponding adjustment of the code will follow:
- Change parameter type to <sal_Int32>
- Bring back the negative value use case
- Adjust the stl vector definition
Comment 13 Oliver-Rainer Wittmann 2012-12-03 11:56:53 UTC
revised solution for former revision 1409212 is in progress
Comment 14 Oliver-Rainer Wittmann 2012-12-03 12:11:43 UTC
Copying SVN Robot comment as I made a typo in the commit message - wrong issue id :(

"orw" committed SVN revision 1416474 into trunk:
#121282# - correction of former solution (revision 1409212) - consider negati...
Comment 15 Oliver-Rainer Wittmann 2012-12-03 12:13:08 UTC
@Pedro:
I made a correction. Can you please verify it. Thx in advance.
Comment 16 Pedro Giffuni 2012-12-03 15:35:27 UTC
(In reply to comment #15)
> @Pedro:
> I made a correction. Can you please verify it. Thx in advance.

Hi;

I tested the correction and it works. Evidently nRowSpan becomes
negative when rows are merged.

Thanks!