You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openoffice.apache.org by bu...@apache.org on 2015/11/08 11:25:11 UTC
[Issue 126635] New: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Issue ID: 126635
Issue Type: DEFECT
Summary: Possible null pointer dereference
Product: General
Version: 4.1.2
Hardware: All
OS: All
Status: UNCONFIRMED
Severity: normal
Priority: P5
Component: code
Assignee: issues@openoffice.apache.org
Reporter: appchecker@cnpo.ru
In file main/sw/source/core/doc/poolfmt.cxx at line 303 there is if-statement:
if( !pColl )
pColl->SetNextTxtFmtColl( *pDoc->GetTxtCollFromPool( nNxt ));
It seems, it is null pointer dereference.
GitHub link:
https://github.com/apache/openoffice/blob/trunk/main/sw/source/core/doc/poolfmt.cxx#L303
The possible defect found by static analyzer AppChecker.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
--- Comment #5 from AppChecker <ap...@cnpo.ru> ---
(In reply to oooforum from comment #1)
> Could you post directly your remark on dev mailing list?
Good afternoon!
Should we send that remark on dev mailing list? IS it necessary since status is
set to CONFIRMED?
One more question: We have found one more possible defect similar to this one.
Should we post it here or send to dev mailing list to?
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
AppChecker <ap...@cnpo.ru> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |appchecker@cnpo.ru
--
You are receiving this mail because:
You are the assignee for the issue.
review denied: [Issue 126635] Possible null pointer dereference :
[Attachment 85136] patch to main/sw/source/core/doc/poolfmt.cxx
Posted by bu...@apache.org.
j.nitschke@ok.de has denied review:
Issue 126635: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Attachment 85136: patch to main/sw/source/core/doc/poolfmt.cxx
https://bz.apache.org/ooo/attachment.cgi?id=85136&action=edit
--- Comment #6 from j.nitschke@ok.de ---
Comment on attachment 85136
--> https://bz.apache.org/ooo/attachment.cgi?id=85136
patch to main/sw/source/core/doc/poolfmt.cxx
thank you for the patch
but I wouldn't approve the current patch
1. use spaces instead of tabs
https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT
https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT/NoTabs
2. the new return path when pColl is NULL breaks functionality
3. as you noted in comment 2, lcl_SetNumBul is a helper of
SwDoc::GetTextCollFromPool and the null check should be there if anywhere
@AppChecker review requests in bugzilla get automatic send to dev-list this has
nothing to do with the issue status
it's done to highlight patches and faster review process
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
--- Comment #7 from orcmid <or...@apache.org> ---
(In reply to j.nitschke from comment #6)
> Comment on attachment 85136 [details]
> patch to main/sw/source/core/doc/poolfmt.cxx
>
> thank you for the patch
> but I wouldn't approve the current patch
> 1. use spaces instead of tabs
> https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT
> https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT/NoTabs
> 2. the new return path when pColl is NULL breaks functionality
> 3. as you noted in comment 2, lcl_SetNumBul is a helper of
> SwDoc::GetTextCollFromPool and the null check should be there if anywhere
>
> @AppChecker review requests in bugzilla get automatic send to dev-list this
> has nothing to do with the issue status
> it's done to highlight patches and faster review process
If you look at the file in SVN, it employs a mix of tabs and blanks and extra
spaces at the ends of lines. I did not cause that.
I did edit the patch to avoid doing all of that tidying at one time. I wanted
to focus on the defect.
I am perfectly willing to remove the null check completely, since we know that
it never happens so long as the function is only executed as a helper locally.
- - - - - - -
It happens that I had to over-ride my editor settings that replace tabs with
spaces (at 4 spaces per tab) and that trim blank ends of lines.
I will clean up that file now with just those fixes.
- - - - - - - - - - -
I disagree about breaking functionality since I presume that pColl == NULL
breaks a pre-condition on the function, so the only way to cope with such a
defect at run-time is to get out as safely as possible.
Note that there was *never* any functionality in recent times -- it was always
returning without accomplishing anything. How is breaking it less a
difficulty, since it has been completely broken for these past years?
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
j.nitschke@ok.de changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |j.nitschke@ok.de
Attachment #85136| |review-
Flags| |
--- Comment #6 from j.nitschke@ok.de ---
Comment on attachment 85136
--> https://bz.apache.org/ooo/attachment.cgi?id=85136
patch to main/sw/source/core/doc/poolfmt.cxx
thank you for the patch
but I wouldn't approve the current patch
1. use spaces instead of tabs
https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT
https://wiki.openoffice.org/wiki/Cpp_Coding_Standards/FORMAT/NoTabs
2. the new return path when pColl is NULL breaks functionality
3. as you noted in comment 2, lcl_SetNumBul is a helper of
SwDoc::GetTextCollFromPool and the null check should be there if anywhere
@AppChecker review requests in bugzilla get automatic send to dev-list this has
nothing to do with the issue status
it's done to highlight patches and faster review process
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Peter <pe...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Resolution|--- |FIXED
CC| |petko@apache.org
Status|ACCEPTED |RESOLVED
--- Comment #19 from Peter <pe...@apache.org> ---
Yes.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
oooforum <oo...@free.fr> changed:
What |Removed |Added
----------------------------------------------------------------------------
Issue Type|DEFECT |PATCH
--
You are receiving this mail because:
You are the assignee for the issue.
review requested: [Issue 126635] Possible null pointer dereference :
[Attachment 85136] patch to main/sw/source/core/doc/poolfmt.cxx
Posted by bu...@apache.org.
orcmid <or...@apache.org> has asked orcmid <or...@apache.org> for review:
Issue 126635: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Attachment 85136: patch to main/sw/source/core/doc/poolfmt.cxx
https://bz.apache.org/ooo/attachment.cgi?id=85136&action=edit
--- Comment #4 from orcmid <or...@apache.org> ---
Created attachment 85136
--> https://bz.apache.org/ooo/attachment.cgi?id=85136&action=edit
patch to main/sw/source/core/doc/poolfmt.cxx
In the unpatched version, the misplaced check on pColl for NULL caused
pColl->SetNextTxtFmtColl( ) to never be executed.
In this patch, the final operation is executed when pColl != NULL.
This may cause unexpected behavior if the failure of this operation is
compensated for anywhere else in the code.
review granted: [Issue 126635] Possible null pointer dereference :
[Attachment 85139] patch to main/sw/source/core/doc/poolfmt.cxx
Posted by bu...@apache.org.
j.nitschke@ok.de has granted review:
Issue 126635: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Attachment 85139: patch to main/sw/source/core/doc/poolfmt.cxx
https://bz.apache.org/ooo/attachment.cgi?id=85139&action=edit
--- Comment #14 from j.nitschke@ok.de ---
Comment on attachment 85139
--> https://bz.apache.org/ooo/attachment.cgi?id=85139
patch to main/sw/source/core/doc/poolfmt.cxx
tested the patch on linux vm. works as intended:
the next style property is set for 'List x Start' and 'List x End' to 'List x'
(this changed)
'List x' and 'List x Cont.' have themselves as next style (stays as before)
'Numbering' formatting work in the same way
btw: next style is applied when you start the next paragraph (enter key)
review requested: [Issue 126635] Possible null pointer dereference :
[Attachment 85139] patch to main/sw/source/core/doc/poolfmt.cxx
Posted by bu...@apache.org.
orcmid <or...@apache.org> has asked for review:
Issue 126635: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Attachment 85139: patch to main/sw/source/core/doc/poolfmt.cxx
https://bz.apache.org/ooo/attachment.cgi?id=85139&action=edit
--- Comment #10 from orcmid <or...@apache.org> ---
Created attachment 85139
--> https://bz.apache.org/ooo/attachment.cgi?id=85139&action=edit
patch to main/sw/source/core/doc/poolfmt.cxx
This modification removes an incorrect test that never passes and that if it
did pass would cause a null-pointer crash.
In all uses of this helper procedure, the pColl parameter is always the value
of pNewColl as allocated and set in the method SwDoc::GetTxtCollFromPool
pNewColl can never be NULL at the time lcl_SetNumBul is called simply because a
NULL value would have causes a crash earlier in GetTxtCollFromPool. So long as
all allocations by *new* throw exceptions and never return NULL pointers, that
is not possible.
ANALYSIS
The first place that could happen is at line 335, if a NULL pointer is obtained
from a table. Assume that entries to the table are always of pointers returned
from successful *new* operations.
Then the ultimate place that pNewColl could be NULL and crash before
lcl_SetNumBul is ever called is at line 383 with a NULL returned by the *new*
operation at whichever of line 375 or 380 is performed.
By default, failed *new* operations throw an exception and do not return any
value.
There is no need to check pNewColl in GetTxtCollFromPool so long as the *new*
implementation used throws exceptions when an allocation/instantiation fails.
NOTE
The consequences of lcl_SetNumBul never accomplishing anything previously is a
greater concern for the safety of this patch. We need to be satisfied that it
suddenly functioning does not have unintended consequences.
I would like broader review than that which led to the previous patch being
unacceptable.
review canceled: [Issue 126635] Possible null pointer dereference :
[Attachment 85136] patch to main/sw/source/core/doc/poolfmt.cxx
Posted by bu...@apache.org.
orcmid <or...@apache.org> has canceled orcmid <or...@apache.org>'s request
for review:
Issue 126635: Possible null pointer dereference
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Attachment 85136: patch to main/sw/source/core/doc/poolfmt.cxx
https://bz.apache.org/ooo/attachment.cgi?id=85136&action=edit
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
orcmid <or...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |orcmid@apache.org
Status|UNCONFIRMED |CONFIRMED
Ever confirmed|0 |1
--- Comment #2 from orcmid <or...@apache.org> ---
Nice one!
This is essentially saying
if (pColl == NULL)
pColl-> ....
an attempt to access a structure at *pColl, when pColl is NULL and OpenOffice
will crash. Hard.
This code was in the original OpenOffice.org code imported for use as the base
for development of Apache OpenOffice in 2011.
There are many places where the containing function, lcl_SetNumBul, is called
with the second parameter presumably not NULL.
Which is to say, the line pColl -> ... is never executed.
It appears that is the actual bug. If a defense against a NULL pColl parameter
is needed, it should be at the beginning and all of the wasted work in this
procedure avoided.
We should then figure out if the line that is never executed needs to be
executed and determine how many issues about OpenOffice behavior are all
attributable to that line not being executed :).
We can also take steps to ensure that the function is truly local to the
SwDoc::GetTextCollFromPool method.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
--- Comment #3 from orcmid <or...@apache.org> ---
(In reply to orcmid from comment #2)
> Which is to say, the line pColl -> ... is never executed.
>
> It appears that is the actual bug. If a defense against a NULL pColl
> parameter is needed, it should be at the beginning and all of the wasted
> work in this procedure avoided.
>
> We should then figure out if the line that is never executed needs to be
> executed and determine how many issues about OpenOffice behavior are all
> attributable to that line not being executed :).
>
> We can also take steps to ensure that the function is truly local to the
> SwDoc::GetTextCollFromPool method.
Since the line in question is never executed, one problem is this: If we now
have it work, what impact will that have elsewhere where having it now work may
be surprising?
Figuring out how to test this will be an interesting challenge.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
orcmid <or...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #85136| |review?(orcmid@apache.org)
Flags| |
--- Comment #4 from orcmid <or...@apache.org> ---
Created attachment 85136
--> https://bz.apache.org/ooo/attachment.cgi?id=85136&action=edit
patch to main/sw/source/core/doc/poolfmt.cxx
In the unpatched version, the misplaced check on pColl for NULL caused
pColl->SetNextTxtFmtColl( ) to never be executed.
In this patch, the final operation is executed when pColl != NULL.
This may cause unexpected behavior if the failure of this operation is
compensated for anywhere else in the code.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
oooforum <oo...@free.fr> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |oooforum@free.fr
--- Comment #1 from oooforum <oo...@free.fr> ---
Could you post directly your remark on dev mailing list?
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
orcmid <or...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Assignee|issues@openoffice.apache.or |orcmid@apache.org
|g |
--- Comment #8 from orcmid <or...@apache.org> ---
(In reply to AppChecker from comment #5)
> One more question: We have found one more possible defect similar to this
> one. Should we post it here or send to dev mailing list to?
Always submit here. This is where there is a single point for
coordinating/addressing the issue. All activity against issues is
automatically echoed to a mailing list, and requests for review go to dev@
automatically.
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Matthias Seidel <ms...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |mseidel@apache.org
--- Comment #18 from Matthias Seidel <ms...@apache.org> ---
As this is committed long ago, can the issue be closed?
--
You are receiving this mail because:
You are the assignee for the issue.
[Issue 126635] Possible null pointer dereference
Posted by bu...@apache.org.
https://bz.apache.org/ooo/show_bug.cgi?id=126635
Keith N. McKenna <kn...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |CLOSED
CC| |knmc@apache.org
--
You are receiving this mail because:
You are the assignee for the issue.