va_start without va_end

classic Classic list List threaded Threaded
8 messages Options
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

va_start without va_end

Hello,

In svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx, I
noticed the use of va_start without va_end.
I read it could create undefined behaviour, so I propose this simple patch.

I propose this patch :
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 3be762d..0ca7223 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -225,6 +225,7 @@ SfxItemSet::SfxItemSet( SfxItemPool& rPool,
              pArgs, sal::static_int_cast< sal_uInt16 >(nWh1),
              sal::static_int_cast< sal_uInt16 >(nWh2),
              sal::static_int_cast< sal_uInt16 >(nNull));
+        va_end(pArgs);
      }
  }

If it's ok, i can commit and push it on master.

Julien.
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Caolán McNamara Caolán McNamara
Reply | Threaded
Open this post in threaded view
|

Re: va_start without va_end

On Wed, 2011-08-03 at 23:56 +0200, Julien Nabet wrote:
> Hello,
>
> In svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx, I
> noticed the use of va_start without va_end.
> I read it could create undefined behaviour, so I propose this simple patch.
>
...
> +        va_end(pArgs);
>       }
>   }
>
> If it's ok, i can commit and push it on master.

The va_end is tucked away hidden inside InitializeRanges_Impl in
svl/source/items/nranges.cxx

This looks rather ugly, it would be nicer to get the va_start and va_end
closer together, e.g. move the va_end out of InitializeRanges_Impl and
put it close to the two va_starts

C.

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: va_start without va_end

Le 04/08/2011 10:33, Caolán McNamara a écrit :

> On Wed, 2011-08-03 at 23:56 +0200, Julien Nabet wrote:
>> Hello,
>>
>> In svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx, I
>> noticed the use of va_start without va_end.
>> I read it could create undefined behaviour, so I propose this simple patch.
>>
> ...
>> +        va_end(pArgs);
>>        }
>>    }
>>
>> If it's ok, i can commit and push it on master.
> The va_end is tucked away hidden inside InitializeRanges_Impl in
> svl/source/items/nranges.cxx
>
> This looks rather ugly, it would be nicer to get the va_start and va_end
> closer together, e.g. move the va_end out of InitializeRanges_Impl and
> put it close to the two va_starts
Hello,

I had took a look at the called function but not at the "called called"
function. Sorry for having missed this.
So I did what you proposed, commited and pushed the patch on master.
I created a tracker for new check on cppcheck about va_start not in
pairs with va_end (ticket 2959)

I found other cases, this time I tried to find the called called...

1)
  /libs-gui/i18npool/source/calendar/
calendar_gregorian.cxx     62 va_start(ap, pat);
59 static void debug_cal_msg(const char *pat, ...)
      60 {
      61     va_list ap;
      62     va_start(ap, pat);
      63     vfprintf(stderr, pat, ap);
      64 }
I read that vfprintf didn't call automatically va_end
(http://www.cplusplus.com/reference/clibrary/cstdio/vfprintf/)
so I would add a va_end there

2)
/libs-core/sfx2/source/menu/
mnumgr.cxx     444 va_start(pArgs, pArg1);
439 sal_uInt16 SfxPopupMenuManager::Execute( const Point& rPoint,
Window* pWindow, const SfxPoolItem *pArg1, ... )
     440 {
     441     DBG_MEMTEST();
     442
     443     va_list pArgs;
     444     va_start(pArgs, pArg1);
     445
     446     return (Execute( rPoint, pWindow, pArgs, pArg1 ));
     447 }

which seems to call in the same file  :
    422 sal_uInt16 SfxPopupMenuManager::Execute( const Point& rPoint,
Window* pWindow, va_list pArgs, const SfxPoolItem *pArg1 )
     423 {
     424     DBG_MEMTEST();
     425
     426     PopupMenu* pPopMenu = ( (PopupMenu*)GetMenu()->GetSVMenu() );
     427     pPopMenu->SetSelectHdl( LINK( this, SfxPopupMenuManager,
SelectHdl ) );
     428     sal_uInt16 nId = pPopMenu->Execute( pWindow, rPoint );
     429     pPopMenu->SetSelectHdl( Link() );
     430
     431     if ( nId )
     432         GetBindings().GetDispatcher()->_Execute( nId,
SFX_CALLMODE_RECORD, pArgs, pArg1 );
     433
     434     return nId;
     435 }

which seems to call in the file
/libs-core/sfx2/source/control/dispatch.cxx :
const SfxPoolItem* SfxDispatcher::_Execute
    1381 (
    1382     sal_uInt16          nSlot,     // the Id of the executing
function
    1383     SfxCallMode         eCall,     // SFX_CALLMODE_SYNCRHON,
..._ASYNCHRON or
    1384                                    //..._SLOT
    1385     va_list             pVarArgs,  // Parameter list from the
2nd parameter
    1386     const SfxPoolItem*  pArg1      // First parameter
    1387 )
    1388
    1389 /*  [Description]
    1390
    1391     Method to excecute a <SfxSlot>s over the Slot-Id.
    1392
    1393     [Return value]
    1394
    1395     const SfxPoolItem*      Pointer to the SfxPoolItem valid to
the next run
    1396                             though the Message-Loop, which
contains the return
    1397                             value.
    1398
    1399                             Or a NULL-Pointer, when the
function was not
    1400                             executed (for example canceled by
the user).
    1401 */
    1402
    1403 {
    1404     if ( IsLocked(nSlot) )
    1405         return 0;
    1406
    1407     SfxShell *pShell = 0;
    1408     const SfxSlot *pSlot = 0;
    1409     if ( GetShellAndSlot_Impl( nSlot, &pShell, &pSlot, sal_False,
    1410                                
SFX_CALLMODE_MODAL==(eCall&SFX_CALLMODE_MODAL) ) )
    1411     {
    1412        SfxAllItemSet aSet( pShell->GetPool() );
    1413
    1414        for ( const SfxPoolItem *pArg = pArg1;
    1415              pArg;
    1416              pArg = va_arg( pVarArgs, const SfxPoolItem* ) )
    1417            MappedPut_Impl( aSet, *pArg );
    1418
    1419        SfxRequest aReq( nSlot, eCall, aSet );
    1420        _Execute( *pShell, *pSlot, aReq, eCall );
    1421        return aReq.GetReturnValue();
    1422     }
    1423     return 0;
    1424 }

so I would add a va_end in SfxPopupMenuManager::Execute

3)
there are some cases (above all in win32 parts that I can't compile to
check patch) like this one :
/components/setup_native/source/win32/customactions/reg64/
reg64.cxx     77 va_start( args, pFormat );
inline void OutputDebugStringFormat( const wchar_t* pFormat, ... )
      73 {
      74     wchar_t    buffer[1024];
      75     va_list args;
      76
      77     va_start( args, pFormat );
      78     StringCchVPrintf( buffer, sizeof(buffer), pFormat, args );
      79     OutputDebugString( buffer );
      80 }
I found nothing which tells if StringCchVPrintf calls automatically
va_end or not.
Since this function seems derived from Vprintf, I would add a va_end too.

Julien
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Norbert Thiebaud Norbert Thiebaud
Reply | Threaded
Open this post in threaded view
|

Re: va_start without va_end

In reply to this post by Caolán McNamara
On Thu, Aug 4, 2011 at 3:33 AM, Caolán McNamara <[hidden email]> wrote:

> On Wed, 2011-08-03 at 23:56 +0200, Julien Nabet wrote:
>> Hello,
>>
>> In svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx, I
>> noticed the use of va_start without va_end.
>> I read it could create undefined behaviour, so I propose this simple patch.
>>
> ...
>> +        va_end(pArgs);
>>       }
>>   }
>>
>> If it's ok, i can commit and push it on master.
>
> The va_end is tucked away hidden inside InitializeRanges_Impl in
> svl/source/items/nranges.cxx

The va_end manpage, on linux says:
"Each invocation of va_start() must be matched by a corresponding
invocation of va_end() in the _same function_"
 (emphasis is mine)
 so it's not just ugly, but also 'wrong'...

Norbert
_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Bjoern Michaelsen Bjoern Michaelsen
Reply | Threaded
Open this post in threaded view
|

SfxItemSet reimplementation (was: va_start without va_end)

Hi Norbert, all,

On Thu, 4 Aug 2011 20:47:43 -0500
Norbert Thiebaud <[hidden email]>
wrote:

> The va_end manpage, on linux says:
> "Each invocation of va_start() must be matched by a corresponding
> invocation of va_end() in the _same function_"
>  (emphasis is mine)
>  so it's not just ugly, but also 'wrong'...

FWIW, I tried a clean new from the group up reimplementation of
SfxItemset once in cws new_itemsets:

 http://hg.services.openoffice.org/cws/new_itemsets/file/924a55dab4dc/svl/inc/svl/itemset.hxx
 http://hg.services.openoffice.org/cws/new_itemsets/file/924a55dab4dc/svl/source/items/itemset.cxx

the intention was to be able to optimize on the code (with seemed to be
impossible with the bit-rotten old one) as SfxItemsets pose a major
part of the load/save-instructions. I never got to integrate that cws,
as its performance currently is just on par with the old implementation
(IIRC, ~8% faster on Linux, but a bit slower on Windows). I still
think it would be a much better base for optimization and finetuning
than the current codebase, let alone be more readable (for example it
is doing the same thing in half the LOC).

Maybe somebody is interested in integrating the replacement Itemset? I
added an EasyHack for it:
 https://bugs.freedesktop.org/show_bug.cgi?id=39849
Although that is a truely challenging task, it would also be most
rewarding.

Best,

Bjoern


--
https://launchpad.net/~bjoern-michaelsen


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: SfxItemSet reimplementation (was: va_start without va_end)

Bjoern Michaelsen wrote
Hi Norbert, all,

On Thu, 4 Aug 2011 20:47:43 -0500
Norbert Thiebaud <[hidden email]>
wrote:

> The va_end manpage, on linux says:
> "Each invocation of va_start() must be matched by a corresponding
> invocation of va_end() in the _same function_"
>  (emphasis is mine)
>  so it's not just ugly, but also 'wrong'...

FWIW, I tried a clean new from the group up reimplementation of
SfxItemset once in cws new_itemsets:
...
Maybe somebody is interested in integrating the replacement Itemset? I
added an EasyHack for it:
 https://bugs.freedesktop.org/show_bug.cgi?id=39849
Although that is a truely challenging task, it would also be most
rewarding.

Best,

Bjoern


--
https://launchpad.net/~bjoern-michaelsen


_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
I don't know if i can do this easyhack but what about the 3 cases of vm_end ? Should vm_end be added ?
If yes, i can commit and pushed them for the 2 first cases. For the third case, i can also do it but I can't check before since it impacts above all Windows parts and I compile on Linux.

Julien.
Eike Rathke Eike Rathke
Reply | Threaded
Open this post in threaded view
|

Re: va_start without va_end

In reply to this post by julien2412
Hi Julien,

On Thursday, 2011-08-04 23:10:08 +0200, Julien Nabet wrote:

>  /libs-gui/i18npool/source/calendar/
> calendar_gregorian.cxx     62 va_start(ap, pat);
> 59 static void debug_cal_msg(const char *pat, ...)
>      60 {
>      61     va_list ap;
>      62     va_start(ap, pat);
>      63     vfprintf(stderr, pat, ap);
>      64 }
> I read that vfprintf didn't call automatically va_end
> (http://www.cplusplus.com/reference/clibrary/cstdio/vfprintf/)
> so I would add a va_end there
Yes please.


> /libs-core/sfx2/source/menu/
> mnumgr.cxx     444 va_start(pArgs, pArg1);
> 439 sal_uInt16 SfxPopupMenuManager::Execute( const Point& rPoint,
> Window* pWindow, const SfxPoolItem *pArg1, ... )
>     440 {
>     441     DBG_MEMTEST();
>     442
>     443     va_list pArgs;
>     444     va_start(pArgs, pArg1);
>     445
>     446     return (Execute( rPoint, pWindow, pArgs, pArg1 ));
>     447 }
>
> which seems to call in the same file  :
>    422 sal_uInt16 SfxPopupMenuManager::Execute( const Point& rPoint,
> Window* pWindow, va_list pArgs, const SfxPoolItem *pArg1 )
>     423 {
>     424     DBG_MEMTEST();
>     425
>     426     PopupMenu* pPopMenu = ( (PopupMenu*)GetMenu()->GetSVMenu() );
>     427     pPopMenu->SetSelectHdl( LINK( this, SfxPopupMenuManager,
> SelectHdl ) );
>     428     sal_uInt16 nId = pPopMenu->Execute( pWindow, rPoint );
>     429     pPopMenu->SetSelectHdl( Link() );
>     430
>     431     if ( nId )
>     432         GetBindings().GetDispatcher()->_Execute( nId,
> SFX_CALLMODE_RECORD, pArgs, pArg1 );
>     433
>     434     return nId;
>     435 }
>
> which seems to call in the file
> /libs-core/sfx2/source/control/dispatch.cxx :
> const SfxPoolItem* SfxDispatcher::_Execute
>    1381 (
>    1382     sal_uInt16          nSlot,     // the Id of the
> executing function
>    1383     SfxCallMode         eCall,     // SFX_CALLMODE_SYNCRHON,
> ..._ASYNCHRON or
>    1384                                    //..._SLOT
>    1385     va_list             pVarArgs,  // Parameter list from
> the 2nd parameter
>    1386     const SfxPoolItem*  pArg1      // First parameter
>    1387 )
>    1388
>    1389 /*  [Description]
>    1390
>    1391     Method to excecute a <SfxSlot>s over the Slot-Id.
>    1392
>    1393     [Return value]
>    1394
>    1395     const SfxPoolItem*      Pointer to the SfxPoolItem valid
> to the next run
>    1396                             though the Message-Loop, which
> contains the return
>    1397                             value.
>    1398
>    1399                             Or a NULL-Pointer, when the
> function was not
>    1400                             executed (for example canceled
> by the user).
>    1401 */
>    1402
>    1403 {
>    1404     if ( IsLocked(nSlot) )
>    1405         return 0;
>    1406
>    1407     SfxShell *pShell = 0;
>    1408     const SfxSlot *pSlot = 0;
>    1409     if ( GetShellAndSlot_Impl( nSlot, &pShell, &pSlot, sal_False,
>    1410
> SFX_CALLMODE_MODAL==(eCall&SFX_CALLMODE_MODAL) ) )
>    1411     {
>    1412        SfxAllItemSet aSet( pShell->GetPool() );
>    1413
>    1414        for ( const SfxPoolItem *pArg = pArg1;
>    1415              pArg;
>    1416              pArg = va_arg( pVarArgs, const SfxPoolItem* ) )
>    1417            MappedPut_Impl( aSet, *pArg );
>    1418
>    1419        SfxRequest aReq( nSlot, eCall, aSet );
>    1420        _Execute( *pShell, *pSlot, aReq, eCall );
>    1421        return aReq.GetReturnValue();
>    1422     }
>    1423     return 0;
>    1424 }
>
> so I would add a va_end in SfxPopupMenuManager::Execute
The one that has va_start, yes.


> 3)
> there are some cases (above all in win32 parts that I can't compile
> to check patch) like this one :
> /components/setup_native/source/win32/customactions/reg64/
> reg64.cxx     77 va_start( args, pFormat );
> inline void OutputDebugStringFormat( const wchar_t* pFormat, ... )
>      73 {
>      74     wchar_t    buffer[1024];
>      75     va_list args;
>      76
>      77     va_start( args, pFormat );
>      78     StringCchVPrintf( buffer, sizeof(buffer), pFormat, args );
>      79     OutputDebugString( buffer );
>      80 }
> I found nothing which tells if StringCchVPrintf calls automatically
> va_end or not.
> Since this function seems derived from Vprintf, I would add a va_end too.
I think so, though I don't know those Windows functions. I'd assume that
any function that takes a va_list does not call va_end itself.
Except our own code ... :-/

  Eike

--
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

_______________________________________________
LibreOffice mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/libreoffice

attachment0 (205 bytes) Download Attachment
julien2412 julien2412
Reply | Threaded
Open this post in threaded view
|

Re: va_start without va_end

I commited and pushed the first and second case.
I let the third case for the moment.

Julien.