Undefined behavior in dpp::populate_result when accessing non-existent headers

[rept1d]

Git commit reference
c9bfdeb

Describe the bug
Here rv.ratelimit_limit, ratelimit_remaining and ratelimit_reset_after are assigned from_string<uint64_t>() of possibly an empty string. As mentioned in the comment, it is the case for non-discord requests.

DPP/src/dpp/queues.cpp

Lines 77 to 82 in c9bfdeb

	/* This will be ignored for non-discord requests without rate limit headers */
	rv.ratelimit_limit = from_string<uint64_t>(res.get_header("x-ratelimit-limit"));
	rv.ratelimit_remaining = from_string<uint64_t>(res.get_header("x-ratelimit-remaining"));
	rv.ratelimit_reset_after = from_string<uint64_t>(res.get_header("x-ratelimit-reset-after"));
	rv.ratelimit_bucket = res.get_header("x-ratelimit-bucket");

However, in such case, contrary to what the comment implies, nothing is ignored. from_string is defined like this:

DPP/include/dpp/stringops.h

Lines 140 to 146 in c9bfdeb

	template <typename T> T from_string(const std::string &s)
	{
	T t;
	std::istringstream iss(s);
	iss >> t;
	return t;
	}

And it will return an indeterminate value if operator >> fails, thus invoking undefined beehaviour. These values will then be assigned to rv.ratelimit_*, and some of them are also used later in conditional statements. For some reason this is already handled correctly for ratelimit_retry_after:

DPP/src/dpp/queues.cpp

Lines 85 to 87 in c9bfdeb

	if (res.get_header("x-ratelimit-retry-after") != "") {
	rv.ratelimit_retry_after = from_string<uint64_t>(res.get_header("x-ratelimit-retry-after"));
	}

Expected behavior
The values remain unchanged (they are previously set to zeroes).

[braindigitalis]

im not sure this is that relavent, these headers are not applicable to non-discord requests? Theyre discord specific headers, the values in the result being undefined values doesnt mean anything.

[rept1d]

Doesn't that apply to dpp::cluster::request and friends? It explicitly supports URLs outside of Discord API.

[braindigitalis]

yes but the value of those headers is not important there as the rate limit code is skipped

[rept1d]

yes but the value of those headers is not important there as the rate limit code is skipped

the problem is not that the values are incorrect but that it's undefined behavior to read them

also, as i mentioned, right after the code in question there are conditional logs based on these values:

DPP/src/dpp/queues.cpp

Lines 95 to 97 in c9bfdeb

	} else if (rv.ratelimit_remaining == 0 && rl_timer > 0) {
	owner->log(ll_debug, "Waiting for endpoint " + url + " rate limit, next request in " + std::to_string(rl_timer) + "s");
	}

we probably don't want them to trigger sporadically

[braindigitalis]

ive been looking further into this, and it does not seem like values that are non-numeric passed through stringstream are uninitialised as you think they are!

Try this out, in https://cpp.sh/

// Example program
#include <iostream>
#include <string>
#include <sstream>

int main()
{
  std::string test{"foo"};
  std::stringstream t(test);
  int a = 42;
  t >> a;
  std::cout << "value: " << a << "!\n"; // outputs 0
}

As you can see the value of a is set to 0 even though it was previously 42, and even though the string contained non-numeric test. Leaving a uninitialised as int a; does the same thing.

Therefore, res.get_headers and from_string<T> never return uninitialised data. As such, i dont think the error is where you think it is. Can you please reconfirm?

[braindigitalis]

further checks show some weirdness here if the string is empty and ONLY if the string is empty. the string is set to 0 if a was uninitialised, and preserved as 42 if the int had a default value. As such i think you may be right and that the 0 is the default value of the memory.

[braindigitalis]

PR created #1087