[vlc-devel] [PATCH] ntservice: fix service command line truncated when using very long arguments

[Hugo Beauzée-Luyssen]

On Thu, Jul 12, 2018, at 5:53 PM, Pierre Lamot wrote:
> ---
>  modules/control/ntservice.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/modules/control/ntservice.c b/modules/control/ntservice.c
> index 995aa9dbb2..2ae7e8565f 100644
> --- a/modules/control/ntservice.c
> +++ b/modules/control/ntservice.c
> @@ -33,6 +33,7 @@
>  #include <vlc_plugin.h>
>  #include <vlc_interface.h>
>  #include <vlc_charset.h>
> +#include <vlc_memstream.h>
>  
>  #define VLCSERVICENAME "VLC media player"
>  
> @@ -180,7 +181,8 @@ static void *Run( void *data )
>  static int NTServiceInstall( intf_thread_t *p_intf )
>  {
>      intf_sys_t *p_sys  = p_intf->p_sys;
> -    char psz_path[10*MAX_PATH], *psz_extra;
> +    char *psz_extra;
> +    struct vlc_memstream path_stream;
>      TCHAR psz_pathtmp[MAX_PATH];
>  
>      SC_HANDLE handle = OpenSCManager( NULL, NULL, SC_MANAGER_ALL_ACCESS );
> @@ -191,39 +193,46 @@ static int NTServiceInstall( intf_thread_t *p_intf )
>          return VLC_EGENERIC;
>      }
>  
> +    if( vlc_memstream_open(&path_stream) != 0 )

Leaking of handle

> +        return VLC_ENOMEM;
> +
>      /* Find out the filename of ourselves so we can install it to the
>       * service control manager */
>      GetModuleFileName( NULL, psz_pathtmp, MAX_PATH );
> -    sprintf( psz_path, "\"%s\" -I ntservice", FromT(psz_pathtmp) );
> +    vlc_memstream_printf( &path_stream, "\"%s\" -I ntservice", 
> FromT(psz_pathtmp) );
>  
>      psz_extra = var_InheritString( p_intf, "ntservice-extraintf" );
>      if( psz_extra && *psz_extra )
>      {
> -        strcat( psz_path, " --ntservice-extraintf " );
> -        strncat( psz_path, psz_extra, MAX_PATH - strlen( psz_path ) - 1 );
> +        vlc_memstream_puts( &path_stream, " --ntservice-extraintf " );
> +        vlc_memstream_puts( &path_stream, psz_extra );

vlc_memstream_printf( "--ntservice-extraintf %s", psz_extra ); 
would be clearer no?

>      }
>      free( psz_extra );
>  
>      psz_extra = var_InheritString( p_intf, "ntservice-options" );
>      if( psz_extra && *psz_extra )
>      {
> -        strcat( psz_path, " " );
> -        strncat( psz_path, psz_extra, MAX_PATH - strlen( psz_path ) - 1 );
> +        vlc_memstream_putc( &path_stream, ' ' );
> +        vlc_memstream_puts( &path_stream, psz_extra );

Same here

>      }
>      free( psz_extra );
>  
> +    if ( vlc_memstream_close( &path_stream ) != 0 )

Also leaking the handle

> +        return VLC_ENOMEM;
> +
>      SC_HANDLE service =
>          CreateServiceA( handle, p_sys->psz_service, p_sys->psz_service,
>                         GENERIC_READ | GENERIC_EXECUTE,
>                         SERVICE_WIN32_OWN_PROCESS,
>                         SERVICE_AUTO_START, SERVICE_ERROR_IGNORE,
> -                       psz_path, NULL, NULL, NULL, NULL, NULL );
> +                       path_stream.ptr, NULL, NULL, NULL, NULL, NULL );
>      if( service == NULL )
>      {
>          if( GetLastError() != ERROR_SERVICE_EXISTS )
>          {
>              msg_Err( p_intf, "could not create new service: \"%s\" (%s)",
> -                     p_sys->psz_service ,psz_path );
> +                     p_sys->psz_service ,path_stream );

Shouldn't this be path_stream.ptr ?

> +            free( path_stream.ptr );
>              CloseServiceHandle( handle );
>              return VLC_EGENERIC;
>          }
> @@ -238,6 +247,8 @@ static int NTServiceInstall( intf_thread_t *p_intf )
>          msg_Warn( p_intf, "service successfuly created" );
>      }
>  
> +    free( path_stream.ptr );
> +
>      if( service ) CloseServiceHandle( service );
>      CloseServiceHandle( handle );
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr