Re: [Libreoffice] [PATCH] possible null-dereferencing found by cppcheck

[Takeshi Abe]

On Tue, 18 Jan 2011 03:26:14 -0700, "Tor Lillqvist" <tlillqv...@novell.com> 
wrote:
> Ah OK. I see that now when I pulled a fresher version.
> 
> But unless I am mistaken, now then pArgs might in theory be de-references 
> while NULL?
> 
> Consider this code path:
> 
>     const SfxItemSet* pArgs = rReq.GetArgs();
> 
>     SFX_REQUEST_ARG (rReq, pHelpLineIndex, SfxUInt32Item, ID_VAL_INDEX, 
> FALSE);
>     // Assume pHelpLineIndex gets set to non-NULL
>    if (pHelpLineIndex != NULL)
>     {
>         // so pArgs gets set to NULL
>         pArgs = NULL;
>     }
>     
>     if ( !pArgs )
>     {
>         // Thus this block is entered
> 
>         SdAbstractDialogFactory* pFact = SdAbstractDialogFactory::Create();
>         // Assume pFact gets set to NULL. Clearly that is possible as the 
> code right after bothers to check for it?
>        AbstractSdSnapLineDlg* pDlg = pFact ? pFact->CreateSdSnapLineDlg( 
> NULL, aNewAttr, mpView ) : 0;
>         // and thus pDlg is NULL
>         if( pDlg )
>         {
>             // so this block is not entered, which is the only place where 
> pArgs gets 
>             // set to non-NULL.
>         }
>     }
>     // Thus pArgs can be NULL here
>     aHlpPos.X() = ((const SfxUInt32Item&) 
> pArgs->Get(ATTR_SNAPLINE_X)).GetValue();
>     aHlpPos.Y() = ((const SfxUInt32Item&) 
> pArgs->Get(ATTR_SNAPLINE_Y)).GetValue();
> 
> Or am I missing something... 
Yes, we have a miserable trap of fall-through:

            switch( nResult )
            {
                case RET_OK:
                    rReq.Done(aNewAttr);
                    pArgs = rReq.GetArgs();
                    break;

                case RET_SNAP_DELETE:
                    // Fangobjekt loeschen
                    if ( !bCreateNew )
                        pPV->DeleteHelpLine(nHelpLine);
                    // und weiter wie bei default
                    /*fall-through*/
                default:
                    return;
            }

Please apply the attached patch saving us in future.

Cheers,
-- Takeshi Abe

>From e560833d8151a0c5745723e0c528c389d5618849 Mon Sep 17 00:00:00 2001
From: Takeshi Abe <t...@fixedpoint.jp>
Date: Tue, 18 Jan 2011 23:22:17 +0900
Subject: [PATCH] mark fall-through

---
 sd/source/ui/func/fusnapln.cxx |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sd/source/ui/func/fusnapln.cxx b/sd/source/ui/func/fusnapln.cxx
index d544ec3..2bc2b01 100644
--- a/sd/source/ui/func/fusnapln.cxx
+++ b/sd/source/ui/func/fusnapln.cxx
@@ -174,6 +174,7 @@ void FuSnapLine::DoExecute( SfxRequest& rReq )
                     if ( !bCreateNew )
                         pPV->DeleteHelpLine(nHelpLine);
                     // und weiter wie bei default
+                    /*fall-through*/
                 default:
                     return;
             }
-- 
1.7.2.3


_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice