On Jul 11, 2013 6:48 AM, "Stefan Sperling" <stsp_at_elego.de> wrote:
>
> I'd like to change an SVN_ERR_MALFUNCTION() in ra_serf into an
> error return, because aborting doesn't help with diagnosing problems.
>
> I also think that we should check for overflow in this case, since
> code further down depends on that not happening. Not very likely,
> since it's an off_t, but still.
>
> Is this patch correct?
>...
> + /* Check for overflow. */
> + if (ctx->read_size + len < ctx->read_size)
> + return svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
> + _("Request response is too large (%"
> + APR_OFF_T_FMT " bytes already read)"),
> + ctx->read_size);
Just make read_size a uint64, and eliminate this check.
> ctx->read_size += len;
>
> if (ctx->skip_size)
> @@ -1654,17 +1660,13 @@ svn_ra_serf__handle_xml_parser(serf_request_t *req
>
> if (ctx->skip_size >= ctx->read_size)
> {
> - /* Eek. What did the file shrink or something? */
> - if (APR_STATUS_IS_EOF(status))
> + /* Eek. What did the file shrink or something? */
> + if (APR_STATUS_IS_EOF(status) ||
APR_STATUS_IS_EAGAIN(status))
If the file shrinks, then EOF is wrong. The caller may think it is done,
when actually it didn't get the whole thing.
This is part of the "resume" logic. If you fetch 2M, lose the connection,
request again, and only get 1.5M ... Problems. It should be (say) 3M, where
we ignore the first 2.
> {
> - SVN_ERR_MALFUNCTION();
> + return svn_ra_serf__wrap_err(status, NULL);
> }
>
> - /* Skip on to the next iteration of this loop. */
> - if (APR_STATUS_IS_EAGAIN(status))
> - {
> - return svn_ra_serf__wrap_err(status, NULL);
> - }
> + /* Skip on to the next iteration of this loop. */
> continue;
EAGAIN is another way to get to the next iteration (by waiting for more
network input). So the comment needs to bind to both EAGAIN and continue.
And EAGAIN is not a "shrink" issue. We just need to wait for more.
IOW, you can combine the return/wrap, but the comments need a bit more work.
Cheers,
-g
Received on 2013-07-11 14:30:24 CEST