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

On Tue, 18 Jan 2011 03:26:14 -0700, "Tor Lillqvist" <tlillqvist@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 <tabe@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