Valgrind reported warnings

[ibc]

By running an application using usrsctp with Valgrind, here the Valgrind warnings. Probably many of them are not important at all. Anyway:

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x100228CEA: sctp_setopt (sctp_usrreq.c:5433)
==52152==    by 0x100236ABC: usrsctp_setsockopt (user_socket.c:2340)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x10022AC16: sctp_select_a_tag (sctputil.c:971)
==52152==    by 0x10022ADEC: sctp_init_asoc (sctputil.c:1085)
==52152==    by 0x1002199BD: sctp_aloc_assoc (sctp_pcb.c:5107)
==52152==    by 0x10022A305: sctpconn_connect (sctp_usrreq.c:8183)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x10021BD1B: sctp_is_vtag_good (sctp_pcb.c:0)
==52152==    by 0x10022AC31: sctp_select_a_tag (sctputil.c:975)
==52152==    by 0x10022ADEC: sctp_init_asoc (sctputil.c:1085)
==52152==    by 0x1002199BD: sctp_aloc_assoc (sctp_pcb.c:5107)
==52152==    by 0x10022A305: sctpconn_connect (sctp_usrreq.c:8183)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x10022AE0F: sctp_init_asoc (sctputil.c:971)
==52152==    by 0x1002199BD: sctp_aloc_assoc (sctp_pcb.c:5107)
==52152==    by 0x10022A305: sctpconn_connect (sctp_usrreq.c:8183)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x100219B31: sctp_aloc_assoc (sctp_pcb.c:5137)
==52152==    by 0x10022A305: sctpconn_connect (sctp_usrreq.c:8183)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F0705: calculate_crc32c (sctp_crc32.c:575)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F0710: calculate_crc32c (sctp_crc32.c:587)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F0726: calculate_crc32c (sctp_crc32.c:574)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F072A: calculate_crc32c (sctp_crc32.c:577)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F0736: calculate_crc32c (sctp_crc32.c:578)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F073E: calculate_crc32c (sctp_crc32.c:591)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F074F: calculate_crc32c (sctp_crc32.c:592)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F075A: calculate_crc32c (sctp_crc32.c:593)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x10020473D: sctp_send_initiate (sctp_output.c:5340)
==52152==    by 0x10022A361: sctpconn_connect (sctp_usrreq.c:8199)
==52152==    by 0x1002366E1: user_connect (user_socket.c:2101)
==52152==    by 0x1002367D7: usrsctp_connect (user_socket.c:2158)

==52152== Use of uninitialised value of size 8
==52152==    at 0x1001F0698: calculate_crc32c (sctp_crc32.c:562)
==52152==    by 0x1001F080B: sctp_calculate_cksum (sctp_crc32.c:0)
==52152==    by 0x1002048FF: sctp_lowlevel_chunk_output (sctp_output.c:5028)
==52152==    by 0x100205CDA: sctp_send_initiate_ack (sctp_output.c:6645)
==52152==    by 0x100200405: sctp_process_control (sctp_input.c:227)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x100215C29: sctp_findassociation_addr (sctp_pcb.c:2530)
==52152==    by 0x1001FAFC8: sctp_common_input_processing (sctp_input.c:5791)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FD8EA: sctp_process_control (sctp_input.c:2680)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FE093: sctp_process_control (sctp_input.c:1670)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FE5DE: sctp_process_control (sctp_input.c:1845)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FE603: sctp_process_control (sctp_input.c:1852)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x10022CB79: sctp_timer_start (sctputil.c:2347)
==52152==    by 0x100202C1E: sctp_start_net_timers (sctp_input.c:939)
==52152==    by 0x1001FEAF1: sctp_process_control (sctp_input.c:2921)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001EDE50: sctp_handle_tick (sctp_callout.c:233)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FFB52: sctp_process_control (sctp_input.c:4919)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s)
==52152==    at 0x1001FE0B4: sctp_process_control (sctp_input.c:1676)
==52152==    by 0x1001FB3F3: sctp_common_input_processing (sctp_input.c:5905)
==52152==    by 0x1002372BF: usrsctp_conninput (user_socket.c:3518)

==52152== Conditional jump or move depends on uninitialised value(s) +31s
==52152==    at 0x10022CB79: sctp_timer_start (sctputil.c:2347)
==52152==    by 0x10022C00E: sctp_timeout_handler (sctputil.c:1866)
==52152==    by 0x1001EDECD: sctp_handle_tick (sctp_callout.c:242)

[tuexen]

Some of them seem to be related to random number generation, where unitialized values might be used intentionally. Others might be bugs. However, can you provide more information? I think valgrind has some setting to provide more information. Are some reproducers available?

[ibc]

I think valgrind has some setting to provide more information

valgrind is already being called with --leak-check=full --track-fds=yes options. I'm not aware of more verbosity options.

BTW logs above are obtained using valgrind for OSX. Now I've tested in Linux and got less than those:

==32408== Conditional jump or move depends on uninitialised value(s)
==32408==    at 0x50D69B: sctp_setopt (sctp_usrreq.c:5433)
==32408==    by 0x4C205F: usrsctp_setsockopt (user_socket.c:2340)

==32408== Conditional jump or move depends on uninitialised value(s)
==32408==    at 0x50D6A9: sctp_setopt (sctp_usrreq.c:5433)
==32408==    by 0x4C205F: usrsctp_setsockopt (user_socket.c:2340)

Are some reproducers available?

We run the demo of our mediasoup project (which uses usrsctp for SCTP/DataChannel support) with valgrind. Did you mean that?

Thanks.

[tuexen]

I think valgrind on MacOS gives more false positives than on Linux. So let's focus on the issue reported on Linux. Could you provide the source code where you call something like usrsctp_setsockopt(..., IPPROTO_SCTP, SCTP_ENABLE_STREAM_RESET, ..., ...). You are providing as the fourth argument a pointer to struct sctp_assoc_value and I guess that you are not initializing the assoc_id component of the structure. Assuming that you are using one-to-one style sockets (generated with SOCK_STREAM), that should be OK since the SCTP stack is required to ignore the assoc_id, but by looking at the code it doesn't do that always. Before providing a fix to test, I would like to confirm my thinking.

[ibc]

Hi, all the usrsctp related code is here. And here an example of usrsctp_setsockopt(). Indeed I do not initialize assoc_id since it's one-to-one style socket (WebRTC DataChannel):

// Set SCTP_ENABLE_STREAM_RESET.
struct sctp_assoc_value av;

av.assoc_value =
  SCTP_ENABLE_RESET_STREAM_REQ | SCTP_ENABLE_RESET_ASSOC_REQ | SCTP_ENABLE_CHANGE_ASSOC_REQ;

ret = usrsctp_setsockopt(this->socket, IPPROTO_SCTP, SCTP_ENABLE_STREAM_RESET, &av, sizeof(av));

[ibc]

AFAIS in stcp_usrreq.c:

#define SCTP_FIND_STCB(inp, stcb, assoc_id) { \
	if ((inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) ||\
	    (inp->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL)) { \
		SCTP_INP_RLOCK(inp); \
		stcb = LIST_FIRST(&inp->sctp_asoc_list); \
		if (stcb) { \
			SCTP_TCB_LOCK(stcb); \
		} \
		SCTP_INP_RUNLOCK(inp); \
	} else if (assoc_id > SCTP_ALL_ASSOC) { \
		stcb = sctp_findassociation_ep_asocid(inp, assoc_id, 1); \
		if (stcb == NULL) { \
			SCTP_LTRACE_ERR_RET(inp, NULL, NULL, SCTP_FROM_SCTP_USRREQ, ENOENT); \
			error = ENOENT; \
			break; \
		} \
	} else { \
		stcb = NULL; \
	} \
}

Such a SCTP_FIND_STCB macro is called from sctp_setopt() (for instance, in line 5420). The macro evaluates certain values of inp->sctp_flags and, if those are not true, it goes to the else if block and there it checks assoc_id which, indeed, it's not set. So here the possible bug.

inp is declared at the beginning of sctp_setopt() as follows:

struct sctp_inpcb *inp = NULL;

inp = (struct sctp_inpcb *)so->so_pcb;

And checking how such a sctp_inpcb struct is filled is too much for me :)

[tuexen]

The above code is correct, I think.

Could you replace https://github.com/sctplab/usrsctp/blob/master/usrsctplib/netinet/sctp_usrreq.c#L5425:

			if ((inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) ||
			    (inp->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL) ||
			    (av->assoc_id == SCTP_FUTURE_ASSOC) ||
			    (av->assoc_id == SCTP_ALL_ASSOC)) {
				SCTP_INP_WLOCK(inp);
				inp->local_strreset_support = (uint8_t)av->assoc_value;
				SCTP_INP_WUNLOCK(inp);
			}
			if ((av->assoc_id == SCTP_CURRENT_ASSOC) ||
			    (av->assoc_id == SCTP_ALL_ASSOC)) {
				SCTP_INP_RLOCK(inp);
				LIST_FOREACH(stcb, &inp->sctp_asoc_list, sctp_tcblist) {
					SCTP_TCB_LOCK(stcb);
					stcb->asoc.local_strreset_support = (uint8_t)av->assoc_value;
					SCTP_TCB_UNLOCK(stcb);
				}
				SCTP_INP_RUNLOCK(inp);
			}

by

			if ((inp->sctp_flags & SCTP_PCB_FLAGS_TCPTYPE) ||
			    (inp->sctp_flags & SCTP_PCB_FLAGS_IN_TCPPOOL) ||
			    ((inp->sctp_flags & SCTP_PCB_FLAGS_UDPTYPE) &&
			     ((av->assoc_id == SCTP_FUTURE_ASSOC) ||
			      (av->assoc_id == SCTP_ALL_ASSOC)))) {
				SCTP_INP_WLOCK(inp);
				inp->local_strreset_support = (uint8_t)av->assoc_value;
				SCTP_INP_WUNLOCK(inp);
			}
			if ((inp->sctp_flags & SCTP_PCB_FLAGS_UDPTYPE) &&
			    ((av->assoc_id == SCTP_CURRENT_ASSOC) ||
			     (av->assoc_id == SCTP_ALL_ASSOC))) {
				SCTP_INP_RLOCK(inp);
				LIST_FOREACH(stcb, &inp->sctp_asoc_list, sctp_tcblist) {
					SCTP_TCB_LOCK(stcb);
					stcb->asoc.local_strreset_support = (uint8_t)av->assoc_value;
					SCTP_TCB_UNLOCK(stcb);
				}
				SCTP_INP_RUNLOCK(inp);
			}

and retest if this fixes this issue?

[ibc]

Confirmed that, with those changes applied, the valgrind Linux warnings no longer happen :)

[tuexen]

Thanks for testing and providing feedback. Working on a fix.

[tuexen]

16163a3 fixes this issue.

[ibc]

Thanks!