One comment that occurred to me as I read over this patch...
> +typedef struct win_xlate_t {
> + apr_pool_t *pool;
> + char *frompage;
> + char *topage;
> + char *sbcs_table;
> +} win_xlate_t;
Storing a pool in this structure throws up a red flag for me...
> +static apr_status_t win_xlate_conv_buffer(win_xlate_t *convset,
> + const char * inbuf,
> + apr_size_t *inbytes_left,
> + char *outbuf,
> + apr_size_t *outbytes_left)
> +{
> + svn_boolean_t fromUTF8 = FALSE;
> + int requiredlen = 0;
> + wchar_t * widebuffer = 0;
> + apr_size_t convertedchars = 0;
> + UINT CP = CP_ACP;
> +
> + if (convset->frompage)
> + {
> + if (apr_strnatcmp (convset->frompage, "UTF-8") == 0)
> + fromUTF8 = TRUE;
> + else
> + CP = atoi(convset->frompage+2);
> + }
> + if ((fromUTF8)&&(convset->topage))
> + {
> + CP = atoi(convset->topage+2);
> + }
> + if (CP == 0)
> + CP = CP_THREAD_ACP;
> +
> + requiredlen = MultiByteToWideChar(fromUTF8 ? CP_UTF8 : CP, 0, inbuf, -1, 0, 0);
> + if (requiredlen == 0)
> + return APR_EINVAL;
> + widebuffer = apr_pcalloc(convset->pool, requiredlen*sizeof(wchar_t));
> + if (widebuffer == 0)
> + return APR_ENOMEM;
And this is why. Won't we be allocating that buffer every time we
call this function, without ever clearing that pool? If we really
need to allocate that buffer each time, we also need to be sure we're
clearing that pool when we're done with it, which means that
convset->pool really should be a specially created subpool just for
that purpose.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Oct 12 21:51:50 2005