On 15/01/2023 20:10, Jeff King wrote:
> The IOCTLFUNCTION option has been deprecated, and generates a compiler
> warning in recent versions of curl. We can switch to using SEEKFUNCTION
> instead. It was added in 2008 via curl 7.18.0; our INSTALL file already
> indicates we require at least curl 7.19.4.
Ah, I didn't even think about this! I knew they were implemented in 7.18.0
and (as I mentioned previously) used the curl version number to switch
between the two sets of 'options'/implementations. Duh! :(
This is much better!
> We have to rewrite the ioctl functions into seek functions. In some ways
> they are simpler (seeking is the only operation), but in some ways more
> complex (the ioctl allowed only a full rewind, but now we can seek to
> arbitrary offsets).
>
> Curl will only ever use SEEK_SET (per their documentation), so I didn't
> bother implementing anything else, since it would naturally be
> completely untested. This seems unlikely to change, but I added an
> assertion just in case.
>
> Likewise, I doubt curl will ever try to seek outside of the buffer sizes
> we've told it, but I erred on the defensive side here, rather than do an
> out-of-bounds read.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> http-push.c | 4 ++--
> http.c | 20 +++++++++-----------
> http.h | 2 +-
> remote-curl.c | 28 +++++++++++++---------------
> 4 files changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/http-push.c b/http-push.c
> index 1b18e775d0..7f71316456 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -203,8 +203,8 @@ static void curl_setup_http(CURL *curl, const char *url,
> curl_easy_setopt(curl, CURLOPT_INFILE, buffer);
> curl_easy_setopt(curl, CURLOPT_INFILESIZE, buffer->buf.len);
> curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLFUNCTION, ioctl_buffer);
> - curl_easy_setopt(curl, CURLOPT_IOCTLDATA, buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKFUNCTION, seek_buffer);
> + curl_easy_setopt(curl, CURLOPT_SEEKDATA, buffer);
> curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_fn);
> curl_easy_setopt(curl, CURLOPT_NOBODY, 0);
> curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, custom_req);
> diff --git a/http.c b/http.c
> index 8a5ba3f477..ca0fe80ddb 100644
> --- a/http.c
> +++ b/http.c
> @@ -157,21 +157,19 @@ size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> return size / eltsize;
> }
>
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
> +int seek_buffer(void *clientp, curl_off_t offset, int origin)
> {
> struct buffer *buffer = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> -
> - case CURLIOCMD_RESTARTREAD:
> - buffer->posn = 0;
> - return CURLIOE_OK;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + if (origin != SEEK_SET)
> + BUG("seek_buffer only handles SEEK_SET");
I didn't even think to check this; as you say, the documentation
claims only to send SEEK_SET, so ... (but this is obviously a
good idea).
> + if (offset < 0 || offset >= buffer->buf.len) {
> + error("curl seek would be outside of buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
I did at least do this! :)
> +
> + buffer->posn = offset;
> + return CURL_SEEKFUNC_OK;
> }
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> diff --git a/http.h b/http.h
> index 3c94c47910..77c042706c 100644
> --- a/http.h
> +++ b/http.h
> @@ -40,7 +40,7 @@ struct buffer {
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
> -curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
> +int seek_buffer(void *clientp, curl_off_t offset, int origin);
>
> /* Slot lifecycle functions */
> struct active_request_slot *get_active_slot(void);
> diff --git a/remote-curl.c b/remote-curl.c
> index 72dfb8fb86..a76b6405eb 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -717,25 +717,23 @@ static size_t rpc_out(void *ptr, size_t eltsize,
> return avail;
> }
>
> -static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> +static int rpc_seek(void *clientp, curl_off_t offset, int origin)
> {
> struct rpc_state *rpc = clientp;
>
> - switch (cmd) {
> - case CURLIOCMD_NOP:
> - return CURLIOE_OK;
> + if (origin != SEEK_SET)
> + BUG("rpc_seek only handles SEEK_SET, not %d", origin);
>
> - case CURLIOCMD_RESTARTREAD:
> - if (rpc->initial_buffer) {
> - rpc->pos = 0;
> - return CURLIOE_OK;
> + if (rpc->initial_buffer) {
> + if (offset < 0 || offset > rpc->len) {
> + error("curl seek would be outside of rpc buffer");
> + return CURL_SEEKFUNC_FAIL;
> }
> - error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> - return CURLIOE_FAILRESTART;
> -
> - default:
> - return CURLIOE_UNKNOWNCMD;
> + rpc->pos = offset;
> + return CURL_SEEKFUNC_OK;
> }
> + error(_("unable to rewind rpc post data - try increasing http.postBuffer"));
> + return CURL_SEEKFUNC_FAIL;
> }
>
> struct check_pktline_state {
> @@ -959,8 +957,8 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
> rpc->initial_buffer = 1;
> curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
> curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
> - curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKFUNCTION, rpc_seek);
> + curl_easy_setopt(slot->curl, CURLOPT_SEEKDATA, rpc);
> if (options.verbosity > 1) {
> fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
> fflush(stderr);
It looks so much better without #ifdef's (or having to worry about
the git-curl-compat.h header file)!
LGTM
ATB,
Ramsay Jones
