out_s3: use-after-free bug in s3_context_destroy

[rittneje]

Bug Report

There is a use-after-free bug in s3_context_destroy. See the Valgrind report from #3706. (Note: This was a pre-existing bug.)

==6876== Thread 2 flb-pipeline:
==6876== Invalid read of size 8
==6876== at 0x18021B: flb_tls_session_destroy (flb_tls.c:394)
==6876== by 0x16EF4C: destroy_conn (flb_upstream.c:425)
==6876== by 0x16F364: flb_upstream_destroy (flb_upstream.c:540)
==6876== by 0x23BE1B: flb_aws_client_destroy (flb_aws_util.c:217)
==6876== by 0x20F9AE: s3_context_destroy (s3.c:315)
==6876== by 0x213F03: cb_s3_exit (s3.c:1528)
==6876== by 0x1585D9: flb_output_exit (flb_output.c:310)
==6876== by 0x168164: flb_engine_shutdown (flb_engine.c:730)
==6876== by 0x167F69: flb_engine_start (flb_engine.c:666)
==6876== by 0x14A6B0: flb_lib_worker (flb_lib.c:605)
==6876== by 0x4E456DA: start_thread (pthread_create.c:463)
==6876== by 0x5FD471E: clone (clone.S:95)
==6876== Address 0x6330d28 is 24 bytes inside a block of size 32 free'd
==6876== at 0x4C32D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6876== by 0x17E9EA: flb_free (flb_mem.h:122)
==6876== by 0x17FBD8: flb_tls_destroy (flb_tls.c:169)
==6876== by 0x20F98B: s3_context_destroy (s3.c:311)
==6876== by 0x213F03: cb_s3_exit (s3.c:1528)
==6876== by 0x1585D9: flb_output_exit (flb_output.c:310)
==6876== by 0x168164: flb_engine_shutdown (flb_engine.c:730)
==6876== by 0x167F69: flb_engine_start (flb_engine.c:666)
==6876== by 0x14A6B0: flb_lib_worker (flb_lib.c:605)
==6876== by 0x4E456DA: start_thread (pthread_create.c:463)
==6876== by 0x5FD471E: clone (clone.S:95)
==6876== Block was alloc'd at
==6876== at 0x4C33B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==6876== by 0x17E9B1: flb_calloc (flb_mem.h:78)
==6876== by 0x17FAE8: flb_tls_create (flb_tls.c:132)
==6876== by 0x2108E6: cb_s3_init (s3.c:536)
==6876== by 0x159D6E: flb_output_init_all (flb_output.c:945)
==6876== by 0x167964: flb_engine_start (flb_engine.c:542)
==6876== by 0x14A6B0: flb_lib_worker (flb_lib.c:605)
==6876== by 0x4E456DA: start_thread (pthread_create.c:463)
==6876== by 0x5FD471E: clone (clone.S:95)

I believe the root cause of the bug is this:

fluent-bit/plugins/out_s3/s3.c

Lines 310 to 316 in b4d0f9b

	if (ctx->client_tls) {
	flb_tls_destroy(ctx->client_tls);
	}
	if (ctx->s3_client) {
	flb_aws_client_destroy(ctx->s3_client);
	}

flb_aws_client_destroy will destroy the upstream, which in turn will destroy any connections, which in turn will destroy any TLS sessions. This destructor requires the flb_tls object.

fluent-bit/src/flb_upstream.c

Lines 421 to 432 in b4d0f9b

	static int destroy_conn(struct flb_upstream_conn *u_conn)
	{
	#ifdef FLB_HAVE_TLS
	if (u_conn->tls_session) {
	flb_tls_session_destroy(u_conn->tls, u_conn);
	}
	#endif
	mk_list_del(&u_conn->_head);
	flb_free(u_conn);
	return 0;
	}

However, since the flb_tls object in question was destroyed first (s3.c:311), this leads to a use-after-free bug.

The fix for this should be simple - just flip the calls to flb_aws_client_destroy(ctx->s3_client) and flb_tls_destroy(ctx->client_tls) in s3_context_destroy.

cc @PettitWesley

[PettitWesley]

Ping @DrewZhang13

[DrewZhang13]

@rittneje thanks for the root cause investigation and fix PR, added a small comment inside code PR