Possible data corruption in User_buffer

[ansiwen]

The write function in User_buffer possibly copies Cstruct values into a list and returns:

mirage-tcpip/src/tcp/user_buffer.ml

Lines 256 to 277 in a7b799c

	let write t datav =
	let l = lenv datav in
	let mss = Int32.of_int (Window.tx_mss t.wnd) in
	match Lwt_dllist.is_empty t.buffer &&
	(l = mss || not (Window.tx_inflight t.wnd)) with
	| false ->
	t.bufbytes <- Int32.add t.bufbytes l;
	List.iter (fun data -> ignore(Lwt_dllist.add_r data t.buffer)) datav;
	if t.bufbytes < mss then
	Lwt.return_unit
	else
	clear_buffer t
	| true ->
	let avail_len = available_cwnd t in
	match avail_len < l with
	| true ->
	t.bufbytes <- Int32.add t.bufbytes l;
	List.iter (fun data -> ignore(Lwt_dllist.add_r data t.buffer)) datav;
	Lwt.return_unit
	| false ->
	let max_size = Window.tx_mss t.wnd in
	transmit_segments ~mss:max_size ~txq:t.txq datav

which basically means it takes ownership, because copies of Cstruct share the data and it's not transparent when they will be flushed. This leads to problems, when code assumes otherwise and reuses the buffers after write returns, like the Faraday library. This leads to issues like this: anmonteiro/gluten#29

I open this issue to clarify the contract conventions (if there are any) for the write function. Is the buffer ownership transferred, or only "borrowed" until write returns?

[haesbaert]

I'm not sure the authors considered this much, but as you can see only the very last case (transmit_segments) is not a clear offender. We should at least document it.

[haesbaert]

Seems we missed this ?
https://github.com/mirage/mirage-flow/blob/e661203b4305b6ea34d625ec725337d854c4ae83/src/mirage_flow.mli#L78-L84

[ansiwen]

Ok, so ownership is transferred. Thanks. So I will close this.