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

[Peter Lundblad <pe...@famlundblad.se>]

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