[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r18644 - trunk/subversion/libsvn_ra_serf

From: Peter Lundblad <peter_at_famlundblad.se>
Date: 2006-02-28 10:07:34 CET

jerenkrantz@tigris.org writes:
> +static const svn_string_t *
> +create_propval(blame_info_t *info)
> +{
> + const svn_string_t *s;
> +
> + /* Include the null term. */
> + s = svn_string_ncreate(info->prop_attr, info->prop_attr_len + 1, info->pool);
> + if (info->prop_base64 == TRUE)
> + {
> + s = svn_base64_decode_string(s, info->pool);
> + }
> + return s;

Doesn't this make the property value contain a superflous NUL
character in the case where the value was not base64 encoded?

> +}
> +
[...]
> + else if (blame_ctx->state &&
> + blame_ctx->state->state == FILE_REVS_REPORT &&
> + strcmp(name.name, "file-rev") == 0)
> + {
> + blame_info_t *info;
> +
> + push_state(blame_ctx, FILE_REV);
> +
> + info = blame_ctx->state->info;
> +
> + info->path = apr_pstrdup(info->pool, find_attr(attrs, "name"));
> + info->rev = SVN_STR_TO_REV(find_attr(attrs, "rev"));

Missing input validation.

> + }
> + else if (blame_ctx->state &&
> + blame_ctx->state->state == FILE_REV)
> + {
> + blame_info_t *info;
> + const char *enc;
> +
> + info = blame_ctx->state->info;
> +
> + if (strcmp(name.name, "rev-prop") == 0)
> + {
> + push_state(blame_ctx, REV_PROP);
> + }
> + else if (strcmp(name.name, "set-prop") == 0)
> + {
> + push_state(blame_ctx, SET_PROP);
> + }
> + if (strcmp(name.name, "remove-prop") == 0)
> + {
> + push_state(blame_ctx, REMOVE_PROP);
> + }
> + else if (strcmp(name.name, "txdelta") == 0)
> + {
> + blame_ctx->file_rev(blame_ctx->file_rev_baton,
> + info->path, info->rev,
> + info->rev_props,
> + &info->txdelta, &info->txdelta_baton,
> + info->prop_diffs, info->pool);

Error leak.

> +
> + info->stream = svn_base64_decode
> + (svn_txdelta_parse_svndiff(info->txdelta, info->txdelta_baton,
> + TRUE, info->pool), info->pool);
> +
> + push_state(blame_ctx, TXDELTA);
> + }
> +
> + switch (blame_ctx->state->state)
> + {
> + case REV_PROP:
> + case SET_PROP:
> + case REMOVE_PROP:
> + info->prop_name = apr_pstrdup(info->pool, find_attr(attrs, "name"));

Crash if name missing.

> + info->prop_attr = NULL;
> + info->prop_attr_len = 0;
> +
> + enc = find_attr(attrs, "encoding");
> + if (enc && strcmp(enc, "base64") == 0)
> + {
> + info->prop_base64 = TRUE;
> + }
> + else
> + {
> + info->prop_base64 = FALSE;
> + }

Using 8 lines for something that could be writte using one line. :-)
Care for us poor readers:-)

[...]
> + {
> + /* no file changes. */
> + if (!info->stream)
> + {
> + blame_ctx->file_rev(blame_ctx->file_rev_baton,
> + info->path, info->rev,
> + info->rev_props,
> + NULL, NULL,
> + info->prop_diffs, info->pool);

Error leak.

[...]
> + else if (cur_state->state == TXDELTA &&
> + strcmp(name.name, "txdelta") == 0)
> + {
> + svn_stream_close(info->stream);

Error leak.

[...]
> +static void XMLCALL
> +cdata_blame(void *userData, const char *data, int len)
> +{
> + blame_context_t *blame_ctx = userData;
> +
> + if (!blame_ctx->state)
> + {
> + return;
> + }
> +
> + switch (blame_ctx->state->state)
> + {
> + case REV_PROP:
> + expand_string(&blame_ctx->state->info->prop_attr,
> + &blame_ctx->state->info->prop_attr_len,
> + data, len, blame_ctx->state->info->pool);
> + break;
> + case TXDELTA:
> + if (blame_ctx->state->info->stream)
> + {
> + apr_size_t ret_len;
> +
> + ret_len = len;
> +
> + svn_stream_write(blame_ctx->state->info->stream, data, &ret_len);

Error leak.

[...]
> +svn_error_t *
> +svn_ra_serf__get_file_revs(svn_ra_session_t *ra_session,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_ra_file_rev_handler_t handler,
> + void *handler_baton,
> + apr_pool_t *pool)
[...]
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("<S:file-revs-report xmlns:S=\"",
> + sizeof("<S:file-revs-report xmlns:S=\"")-1,
> + session->bkt_alloc);

Indentation and spaces around operator.

[...]
> + SVN_ERR(context_run_wait(&blame_ctx->done, session, pool));
> +
> + return blame_ctx->error;
> +}

We usually destroy subpools in the success code path. *Maybe* you
should traverse the free states list to do this, although I don't
think it is necessary.

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 28 10:11:23 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.