-K/--config does not work correctly with the --next option #5120

Summary

The handling of the linked list of OperationConfig structures is broken in both parseconfig() and parseargs(), leading to a number of bugs.

1. parseconfig() (defined in curl/tool_parsecfg.c:74) initializes

   (line 79): struct OperationConfig *operation = global->first;

   If the config file being parsed by parseconfig contains --next, a new OperationConfig will be installed:

   (line 244): operation->next = malloc(sizeof(struct OperationConfig));

   However, operation is not at the end of the linked list; it's at the beginning. If operation->next already exists (i.e. there were already at least two operations), it will be overwritten and it and the linked list of operations it points to will be lost (and the memory allocated to them will leak).

   To make this work, operation should have been initialised to global->last.

2. parse_args() (defined in src/tool_getparam.c:2237) calls getparameter() at line 2260. But getparameter might call parseconfig() (if -K/--config appears on the command line). Independently of issue (1), parseconfig() may add a new OperationConfig to the linked list, so when parse_args() returns, its config local variable no longer points at the currently active OperationConfig. (Indeed, this is also true inside of getparameter() after parseconfig() returns, but I've convinced myself that config is not actually used after than point. Still, it maybe should be fixed.)

Suggested fixes

1. Change curl/tool_parsecfg.c:79 to

    struct OperationConfig *operation = global->last;
   

2. After the call to getparameter() at src/tool_getparam.c:2260, insert

    config = global->last;
   

   It's possible that this also needs to be done after the call to parseconfig() at src/tool_getparam.c:1842.

3. As far as I can see, nothing stops a config file from including a recursive -K/--config option. To handle that possibility, insert

    operation = global->last;
   

   after the call to getparameter() at src/tool_parsecfg.c:235.

As an alternative to (2) and (3), consideration should be given to making the config argument of getparameter() an "in-out" parameter by adding an extra level of indirection. In that case, it would make sense to add a similar "in-out" parameter to parseconfig.

Below, you'll find some tests.

I did this

First, I set up a simple HTTP server which echos back the request and any headers starting T_ (to avoid too much output). Then I wrote the following simple bash function which generates a request in config-file format, starting with --next. Note that since none of the option values includes whitespace, it's possible to also use the same script to produce command-line arguments:

req () { 
    printf -- "--next\n-H T_$1:$1\n--url http://localhost:8080/$1\n"
}

$ req one
--next
-H T_one:one
--url http://localhost:8080/one
$ req two
--next
-H T_two:two
--url http://localhost:8080/two

Invoking curl using command-line arguments works as expected:

$ curl $(req one) $(req two) $(req three) $(req four)
{
  "request": "GET /one",
  "headers": {
    "T_one": "one"
  }
}
{
  "request": "GET /two",
  "headers": {
    "T_two": "two"
  }
}
{
  "request": "GET /three",
  "headers": {
    "T_three": "three"
  }
}
{
  "request": "GET /four",
  "headers": {
    "T_four": "four"
  }
}

But when the -K option is used, these fail in various ways.

If all of the request are in config files, then only the first and the last are processed. (The second and third are successively overwritten as described above.)

$ curl -K <(req one) -K <(req two) -K <(req three) -K <(req four)  
{
  "request": "GET /one",
  "headers": {
    "T_one": "one"
  }
}
{
  "request": "GET /four",
  "headers": {
    "T_four": "four"
  }
}

Other failure modes occur when config files are mixed with requests on the command line, because the OperationConfig being filled in by parse_args is not advanced when after the -K option is processed. This results in the second command-line request overwriting preceding config file requests. (This is a similar symptom to the first problem, but the cause is different.)

$ curl $(req one) -K <(req two) -K <(req three) $(req four)
{
  "request": "GET /one",
  "headers": {
    "T_one": "one"
  }
}
{
  "request": "GET /four",
  "headers": {
    "T_four": "four"
  }
}

$ curl $(req one) -K <(req two) $(req three) $(req four)
{
  "request": "GET /one",
  "headers": {
    "T_one": "one"
  }
}
{
  "request": "GET /three",
  "headers": {
    "T_three": "three"
  }
}
{
  "request": "GET /four",
  "headers": {
    "T_four": "four"
  }
}

This can also result in options at the end of the command line being applied in surprising ways in certain circumstances, if not preceded on the command-line with --next:

$ curl -K <(req one) -K <(req two) -H T_surprise:'!'
{
  "request": "GET /one",
  "headers": {
    "T_one": "one",
    "T_surprise": "!"
  }
}
{
  "request": "GET /two",
  "headers": {
    "T_two": "two"
  }
}

$ curl -K <(req one) -K <(req two) -H T_surprise:'!' http://localhost:8080/surprise
{
  "request": "GET /one",
  "headers": {
    "T_one": "one",
    "T_surprise": "!"
  }
}
{
  "request": "GET /surprise",
  "headers": {
    "T_one": "one",
    "T_surprise": "!"
  }
}
{
  "request": "GET /two",
  "headers": {
    "T_two": "two"
  }
}

I expected the following

I expected all of the four-request command lines above to produce exactly the same output, and for the "surprise" headers to be added to the last config-file, not the first one. (Had the first request been a command-line request, these headers would have been added to it rather than the config-file request. That might be considered less surprising, but it's still surprising.)

curl/libcurl version

Tested with

$ curl --version
curl 7.58.0 (x86_64-pc-linux-gnu) libcurl/7.58.0 OpenSSL/1.1.1 zlib/1.2.11 libidn2/2.0.4 libpsl/0.19.1 (+libidn2/2.0.4) nghttp2/1.30.0 librtmp/2.3
Release-Date: 2018-01-24
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL 

and also with the latest tarball

$ src/curl --version
curl 7.69.1 (x86_64-pc-linux-gnu) libcurl/7.69.1 OpenSSL/1.1.1 zlib/1.2.11
Release-Date: 2020-03-11
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS HTTPS-proxy IPv6 Largefile libz NTLM NTLM_WB SSL TLS-SRP UnixSockets

But I think the behaviour dates back to this commit in early 2014: fc59a9e

operating system

Tested on

Linux rici-hp 4.15.0-32-generic #35-Ubuntu SMP Fri Aug 10 17:58:07 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

But it's probably cross-platform.