urlapi: stricter CURLUPART_PORT parsing

[bagder]

Only allow well formed decimal numbers in the input.

Document that the number MUST be between 1 and 65535.

Add tests to test 1560 to verify the above.

Ref: #3753

[danielgustafsson]

LGTM

[bagder]

Nice fix @jay. I think we should also add a test for that case. I can do that tomorrow.

[jay]

I might not have gone far enough. The reporter in #3753 basically showed that CURLOPT_PORT 1234L set that port in his curlu but CURLOPT_PORT 0L didn't reset it to the default port. Of course neither one of those things should have had any effect on his curlu and you fixed that in #3761. Separately from that however is still the issue of how it is treated internally. The reporter repro boiled down is basically this:

curl_easy_setopt(curl, CURLOPT_CURLU, curlu);
curl_easy_setopt(curl, CURLOPT_PORT, 1234L);
curl_code = curl_easy_perform(curl);
// now reset port to go back to port 80 (not!)
curl_easy_setopt(curl, CURLOPT_PORT, 0L);
curl_code = curl_easy_perform(curl);

Internally parse_remote_port() will be called in the first case to update the internal CURLU (ie the CURLU that would be copied from the user's curlu in this example). Yet that does not happen in the second case since data->set.use_port is 0:

curl/lib/url.c

Lines 3009 to 3025 in 521bbbe

	static CURLcode parse_remote_port(struct Curl_easy *data,
	struct connectdata *conn)
	{
	if(data->set.use_port && data->state.allow_port) {
	/* if set, we use this instead of the port possibly given in the URL */
	char portbuf[16];
	CURLUcode uc;
	conn->remote_port = (unsigned short)data->set.use_port;
	msnprintf(portbuf, sizeof(portbuf), "%d", conn->remote_port);
	uc = curl_url_set(data->state.uh, CURLUPART_PORT, portbuf, 0);
	if(uc)
	return CURLE_OUT_OF_MEMORY;
	}
	return CURLE_OK;
	}

My fix would allow the user to use curl_url_set(uh, CURLUPART_PORT, "", 0); to reset the port in their own CURLU (which is read-only for us) but doesn't address this with the internal CURLU. In other words the user still can't use CURLOPT_PORT 0L to reset the port in the URL AFAICS (granted I haven't actually tried this yet).

[bagder]

I might not have gone far enough.

I think you're overthinking it. This PR and change is only about how to parse the CURLUPART_PORT string when curl_url_set is called.

The #3753 issue was totally separate. It occurred primarily because curl modified the handle that it wasn't supposed to touch so even when setting CURLOPT_PORT to zero, the port number in the URL wasn't correctly reset. That part was fixed in #3761.

[bagder]

Oh sorry, this extra "" support is unnecessary. The man page actually already says:

Setting a part to a NULL pointer will effectively remove that part's contents from the CURLU handle.

So NULL, not "", has that effect. I'll add a test case for it.

[jay]

Oops. I also missed that in the code and now it seems totally obvious. Does portnum need to be cleared:

diff --git a/lib/urlapi.c b/lib/urlapi.c
index 3da8ff8..0eb06d2 100644
--- a/lib/urlapi.c
+++ b/lib/urlapi.c
@@ -1145,6 +1145,7 @@ CURLUcode curl_url_set(CURLU *u, CURLUPart what,
       storep = &u->host;
       break;
     case CURLUPART_PORT:
+      u->portnum = 0;
       storep = &u->port;
       break;
     case CURLUPART_PATH:

[bagder]

yes!