CDRIVER-6151 replaced mcd-time.h with mlib utilities

[kevinAlbs]

Motivated by comments in #2166. This PR replaces use of mcd-time.h with newer mlib utilities.

Patch build: https://spruce.mongodb.com/version/6913867bc439ef000796cc85

[kevinAlbs]

Updated.

[kevinAlbs]

I expect assigning time_since_monotonic_start to the result of bson_gettimeofday may not be right. time_since_monotonic_start uses the system monotonic clock, which I expect is not necessarily a Unix time. Instead: updated to use mlib operations to do checked arithmetic.

Also consider defining MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW as a typed duration.

Good idea. Done.

[eramongodb]

Suggested change

	// Prevent passing zero as a timeout
	mlib_time_cmp(timer.expires_at, >, (mlib_time_point){0}) ? _mongoc_http_msec_remaining(timer) : 1,
	// Prevent passing zero as a timeout
	BSON_MAX(_mongoc_http_msec_remaining(timer), 1),

Sorry, my prior suggestion was incorrect: this mixes up the expires_at time point with a duration.

[kevinAlbs]

Updated to use BSON_MAX to simplify. But I expect the prior suggestion was still OK (timer.expires_at is an mlib_time_point). And mlib_time_cmp compares to mlib_time_point.

[eramongodb]

Suggested change

	#define MONGOC_OCSP_REQUEST_TIMEOUT mlib_duration(5000, ms)
	#define MONGOC_OCSP_REQUEST_TIMEOUT mlib_duration(5, s)

Reduction.

[eramongodb]

Suggested change

	if (mlib_mul(&now_ms, 100, now.tv_sec) || mlib_add(&now_ms, now.tv_usec / 1000) ||
	if (mlib_mul(&now_ms, 1000, now.tv_sec) || mlib_add(&now_ms, now.tv_usec / 1000) ||

Missing a 0?

Using mlib_duration may still help avoid unit errors (even if we choose not to use mlib_time_point as in the initial suggestion due to clock consistency concerns).

static bool expiration_ms_to_timer(int64_t expiration_ms, mlib_timer *expiration_timer, bson_error_t *error) { bool ret = false; const mlib_duration expires_in = mlib_duration((expiration_ms, ms), minus, MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW); mlib_duration now = {0}; // current time in milliseconds since Unix Epoch. { struct timeval tv; if (0 != bson_gettimeofday(&tv)) { AUTH_ERROR_AND_FAIL("bson_gettimeofday returned failure. Unable to " "determine expiration."); } else { now = mlib_duration((tv.tv_sec, s), plus, (tv.tv_usec, us)); } } *expiration_timer = mlib_expires_after(expires_in, minus, now); ret = true; fail: return ret; }

[kevinAlbs]

Thank you for suggestion. Applied with tweaks to (hopefully) clarify which are times since the Unix epoch.

This function's implementation is all sorts of strange to me. The original code computes the expiration time point as (simplified):

mcd_now() + ((expiration_ms - now_ms) - MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS);

As I understand it:

• expiration_ms is relative to the Unix epoch (not comparable to the clock used for .time_since_monotonic_start).
• now_ms is milliseconds since Unix epoch.
• expiration_ms - now_ms is a duration.
• (expiration_ms - now_ms) - MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS subtracts 5 minutes to meet the spec requirement "considered valid if they are more than five minutes away from expiring".

[eramongodb]

This function's implementation is all sorts of strange to me. The original code computes the expiration time point as (simplified):

mcd_now() + ((expiration_ms - now_ms) - MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS);

The function description states:

expiration_ms_to_timer converts milliseconds since Unix Epoch into the mlib_timer expiration_timer

This means expiration_ms is a .time_since_monotonic_start in units of milliseconds. This time point is being subtracted with bson_gettimeofday() (clock confusion?) that is manually converted into a .time_since_monotonic_start in units of milliseconds. This difference (after applying MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS) is then treated as the timeout duration relative to mcd_now() -> bson_get_monotonic_time() -> mlib_now(). So the clock confusion between mlib_now() and bson_gettimeofday() has already been present.

If not addressed in this PR, I think this code definitely deserves its own (re-)review apart from this PR.

[kevinAlbs]

Updated.

[kevinAlbs]

Thank you for suggestion. Applied with tweaks to (hopefully) clarify which are times since the Unix epoch.

This function's implementation is all sorts of strange to me. The original code computes the expiration time point as (simplified):

mcd_now() + ((expiration_ms - now_ms) - MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS);

As I understand it:

• expiration_ms is relative to the Unix epoch (not comparable to the clock used for .time_since_monotonic_start).
• now_ms is milliseconds since Unix epoch.
• expiration_ms - now_ms is a duration.
• (expiration_ms - now_ms) - MONGOC_AWS_CREDENTIALS_EXPIRATION_WINDOW_MS subtracts 5 minutes to meet the spec requirement "considered valid if they are more than five minutes away from expiring".

[kevinAlbs]

Updated to use BSON_MAX to simplify. But I expect the prior suggestion was still OK (timer.expires_at is an mlib_time_point). And mlib_time_cmp compares to mlib_time_point.