Re: svn commit: r1920871 - in /apr/apr/trunk: CHANGES test/testproc.c threadproc/win32/proc.c

[Ruediger Pluem <rp...@apache.org>]

On 9/24/24 8:26 AM, ivan@apache.org wrote:
> Author: ivan
> Date: Tue Sep 24 06:26:21 2024
> New Revision: 1920871
> 
> URL: http://svn.apache.org/viewvc?rev=1920871&view=rev
> Log:
> apr_proc_create(): Check that progname argument is quoted correctly if
> it's quoted on Windows.
> 
> Modified:
>     apr/apr/trunk/CHANGES
>     apr/apr/trunk/test/testproc.c
>     apr/apr/trunk/threadproc/win32/proc.c
> 
> Modified: apr/apr/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1920871&r1=1920870&r2=1920871&view=diff
> ==============================================================================
> --- apr/apr/trunk/CHANGES [utf-8] (original)
> +++ apr/apr/trunk/CHANGES [utf-8] Tue Sep 24 06:26:21 2024
> @@ -297,6 +297,9 @@ Changes for APR 2.0.0
>    *) apr_proc_create(): Fix potential handle leak when apr_proc_create() is used
>       from multiple threads on Windows [Ivan Zhakov]
>  
> +  *) apr_proc_create(): Check that progname argument is quoted correctly if
> +     it's quoted on Windows. [Ivan Zhakov]
> +
>  Changes for APR and APR-util 1.7.x and later:
>  
>    *) http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/CHANGES?view=markup
> 
> Modified: apr/apr/trunk/test/testproc.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testproc.c?rev=1920871&r1=1920870&r2=1920871&view=diff
> ==============================================================================
> --- apr/apr/trunk/test/testproc.c (original)
> +++ apr/apr/trunk/test/testproc.c Tue Sep 24 06:26:21 2024
> @@ -300,6 +300,34 @@ static void test_proc_args_winbatch(abts
>      ABTS_STR_EQUAL(tc, expected, actual);
>  }
>  
> +#ifdef WIN32
> +static void test_proc_unclosed_quote1(abts_case *tc, void *data)
> +{
> +    apr_procattr_t *attr;
> +    apr_status_t rv;
> +    const char *args[] = { NULL };
> +
> +    rv = apr_procattr_create(&attr, p);
> +    ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
> +
> +    rv = apr_proc_create(&newproc, "\"", args, NULL, attr, p);
> +    ABTS_INT_EQUAL(tc, APR_EINVAL, rv);
> +}
> +
> +static void test_proc_unclosed_quote2(abts_case *tc, void *data)
> +{
> +    apr_procattr_t *attr;
> +    apr_status_t rv;
> +    const char *args[] = { NULL };
> +
> +    rv = apr_procattr_create(&attr, p);
> +    ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
> +
> +    rv = apr_proc_create(&newproc, "\"abc", args, NULL, attr, p);
> +    ABTS_INT_EQUAL(tc, APR_EINVAL, rv);
> +}
> +#endif
> +
>  abts_suite *testproc(abts_suite *suite)
>  {
>      suite = ADD_SUITE(suite)
> @@ -311,6 +339,8 @@ abts_suite *testproc(abts_suite *suite)
>      abts_run_test(suite, test_proc_args, NULL);
>  #ifdef WIN32
>      abts_run_test(suite, test_proc_args_winbatch, NULL);
> +    abts_run_test(suite, test_proc_unclosed_quote1, NULL);
> +    abts_run_test(suite, test_proc_unclosed_quote2, NULL);
>  #endif
>  
>      return suite;
> 
> Modified: apr/apr/trunk/threadproc/win32/proc.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/win32/proc.c?rev=1920871&r1=1920870&r2=1920871&view=diff
> ==============================================================================
> --- apr/apr/trunk/threadproc/win32/proc.c (original)
> +++ apr/apr/trunk/threadproc/win32/proc.c Tue Sep 24 06:26:21 2024
> @@ -509,7 +509,16 @@ APR_DECLARE(apr_status_t) apr_proc_creat
>       * XXX progname must be NULL if this is a 16 bit app running in WOW
>       */
>      if (progname[0] == '\"') {
> -        progname = apr_pstrmemdup(pool, progname + 1, strlen(progname) - 2);
> +        size_t progname_len = strlen(progname);
> +        if (progname_len < 2) {
> +            return APR_EINVAL;
> +        }
> +
> +        if (progname[progname_len - 1] != '\"') {
> +            return APR_EINVAL;
> +        }

Shouldn't we ensure that there is no '\"' in the middle of the string, e.g

if (strchr(progname + 1, '\"') != progname + progname_len - 1) {
    return APR_EINVAL;
}

?

Regards

Rüdiger