Bug 7159 - client rpc_transport doesn't cope with bad server data returns.
Summary: client rpc_transport doesn't cope with bad server data returns.
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.5
Classification: Unclassified
Component: Client Tools (show other bugs)
Version: 3.5.0rc3
Hardware: Other Linux
: P3 normal
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 7295
  Show dependency treegraph
 
Reported: 2010-02-19 15:56 UTC by Jeremy Allison
Modified: 2020-10-02 11:50 UTC (History)
2 users (show)

See Also:


Attachments
git-am fix for 3.5.x - part1 (2.73 KB, patch)
2010-02-19 17:03 UTC, Jeremy Allison
metze: review+
Details
git-am fix for 3.5.x - part2 (1.09 KB, patch)
2010-02-19 17:04 UTC, Jeremy Allison
metze: review+
Details
Should we add a patch like this to master? (1.67 KB, patch)
2010-02-20 02:13 UTC, Stefan Metzmacher
no flags Details
Patch for v3-4 (3.88 KB, patch)
2010-04-06 08:21 UTC, Stefan Metzmacher
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2010-02-19 15:56:13 UTC
Deliberately break the rpc_server in master using the following patch:

--- a/source3/rpc_server/srv_pipe.c
+++ b/source3/rpc_server/srv_pipe.c
@@ -1936,7 +1936,7 @@ bool api_pipe_bind_req(pipes_struct *p, prs_struct *rpc_in_p)
        init_rpc_hdr(&p->hdr, DCERPC_PKT_BIND_ACK, DCERPC_PFC_FLAG_FIRST | DCERPC_PFC_FLAG_LAST,
                        p->hdr.call_id,
                        RPC_HEADER_LEN + prs_offset(&out_hdr_ba) +
-                               ss_padding_len + prs_offset(&out_auth),
+                               4 + prs_offset(&out_auth),
                        auth_len);
 
        /*

This causes the DCE fragment length to be reported as 4 bytes larger than the actual data being returned.

Then call:

valgrind --num-callers=40 bin/smbclient -L 127.0.0.1 -U%

to get:

==14875== Memcheck, a memory error detector
==14875== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==14875== Using Valgrind-3.5.0-Debian and LibVEX; rerun with -h for copyright info
==14875== Command: bin/smbclient -L //127.0.0.1 -Ujra%pxr5@USOM
==14875== 
Domain=[MSHOME] OS=[Unix] Server=[Samba 3.6.0-GIT-af4a7c0-devel]

	Sharename       Type      Comment
	---------       ----      -------
==14875== Invalid read of size 4
==14875==    at 0x871FA47: talloc_chunk_from_ptr (talloc.c:231)
==14875==    by 0x8720BB2: talloc_get_name (talloc.c:937)
==14875==    by 0x8720D23: _talloc_get_type_abort (talloc.c:990)
==14875==    by 0x80ED562: rpc_np_read_done (rpc_transport_np.c:153)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C3934: _tevent_req_error (tevent_req.c:198)
==14875==    by 0x82795EA: tevent_req_nterror (tevent_ntstatus.c:25)
==14875==    by 0x814ADDD: cli_read_andx_done (clireadwrite.c:171)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C3BC0: tevent_req_timedout (tevent_req.c:361)
==14875==    by 0x82BFD62: run_events (events.c:128)
==14875==    by 0x82C0049: s3_event_loop_once (events.c:192)
==14875==    by 0x82C11EC: _tevent_loop_once (tevent.c:497)
==14875==    by 0x82C3AEA: tevent_req_poll (tevent_req.c:329)
==14875==    by 0x80E9AB7: rpc_pipe_bind (cli_pipe.c:3056)
==14875==    by 0x80EB841: cli_rpc_pipe_open_noauth_transport (cli_pipe.c:3798)
==14875==    by 0x80EBA86: cli_rpc_pipe_open_noauth (cli_pipe.c:3831)
==14875==    by 0x80D4BEB: browse_host_rpc (client.c:3712)
==14875==    by 0x80D4EA7: browse_host (client.c:3764)
==14875==    by 0x80D6CE9: do_host_query (client.c:4620)
==14875==    by 0x80D81F7: main (client.c:5052)
==14875==  Address 0x414fe08 is 32 bytes inside a block of size 104 free'd
==14875==    at 0x4007836: free (vg_replace_malloc.c:325)
==14875==    by 0x87203F6: _talloc_free_internal (talloc.c:669)
==14875==    by 0x8721032: _talloc_free (talloc.c:1133)
==14875==    by 0x80E1989: rpc_read_done (cli_pipe.c:338)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C38E4: _tevent_req_done (tevent_req.c:158)
==14875==    by 0x80ED697: rpc_np_read_done (rpc_transport_np.c:183)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C38E4: _tevent_req_done (tevent_req.c:158)
==14875==    by 0x814B100: cli_read_andx_done (clireadwrite.c:206)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C38E4: _tevent_req_done (tevent_req.c:158)
==14875==    by 0x816A318: cli_smb_received (async_smb.c:712)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C38E4: _tevent_req_done (tevent_req.c:158)
==14875==    by 0x8168BA7: read_smb_done (async_smb.c:93)
==14875==    by 0x82C388F: _tevent_req_notify_callback (tevent_req.c:134)
==14875==    by 0x82C38BE: tevent_req_finish (tevent_req.c:143)
==14875==    by 0x82C38E4: _tevent_req_done (tevent_req.c:158)
==14875==    by 0x8286FE7: read_packet_handler (async_sock.c:593)
==14875==    by 0x82BFE42: run_events (events.c:148)
==14875==    by 0x82C0157: s3_event_loop_once (events.c:211)
==14875==    by 0x82C11EC: _tevent_loop_once (tevent.c:497)
==14875==    by 0x82C3AEA: tevent_req_poll (tevent_req.c:329)
==14875==    by 0x80E9AB7: rpc_pipe_bind (cli_pipe.c:3056)
==14875==    by 0x80EB841: cli_rpc_pipe_open_noauth_transport (cli_pipe.c:3798)
==14875==    by 0x80EBA86: cli_rpc_pipe_open_noauth (cli_pipe.c:3831)
==14875==    by 0x80D4BEB: browse_host_rpc (client.c:3712)
==14875==    by 0x80D4EA7: browse_host (client.c:3764)
==14875==    by 0x80D6CE9: do_host_query (client.c:4620)
==14875==    by 0x80D81F7: main (client.c:5052)

This is caused by a missing TALLOC_FREE on a req that has a tevent timeout attached. Once the timeout fires (after 30 seconds) it is invoked on a request with a pointer to already freed data.

This is problem 1. Problem 2. If that the client rpc transport doesn't read a zero byte return as EOF, but keeps issuing read requests (pipelined very efficiently :-) to try and get those extra 4 bytes which will never come.

2 patches to follow.

Jeremy.
Comment 1 Jeremy Allison 2010-02-19 17:03:54 UTC
Created attachment 5401 [details]
git-am fix for 3.5.x - part1
Comment 2 Jeremy Allison 2010-02-19 17:04:45 UTC
Created attachment 5402 [details]
git-am fix for 3.5.x - part2
Comment 3 Jeremy Allison 2010-02-19 17:05:59 UTC
Metze, Volker. Let me know if you think this is 3.5.0 material or for 3.5.1.

My personal feeling is that 3.5.1 would be fine.

Jeremy.
Comment 4 Stefan Metzmacher 2010-02-20 02:10:02 UTC
Comment on attachment 5402 [details]
git-am fix for 3.5.x - part2

Looks good
Comment 5 Stefan Metzmacher 2010-02-20 02:10:07 UTC
Comment on attachment 5401 [details]
git-am fix for 3.5.x - part1

Looks good
Comment 6 Stefan Metzmacher 2010-02-20 02:11:20 UTC
I would be fine with 3.5.0, but also with 3.5.1
Comment 7 Stefan Metzmacher 2010-02-20 02:13:38 UTC
Created attachment 5403 [details]
Should we add a patch like this to master?

I'm not sure, but maybe we should add something like this patch to master,
I assume it's not strictly needed for 3.5.x.
Comment 8 Stefan Metzmacher 2010-02-23 02:49:47 UTC
Jeremy please comment on my patch for master
and reassign the bug to Karolin to get your patches into the release
Comment 9 Jeremy Allison 2010-02-23 11:43:10 UTC
+1 on your patch for master. Makes sense.
Re-assigning to Karolin for inclusion.

Jeremy.
Comment 10 Karolin Seeger 2010-02-24 09:14:34 UTC
I will include the patches in 3.5.1.
Comment 11 Jeremy Allison 2010-02-24 11:37:11 UTC
Thanks Karolin, I also think targetting 3.5.1 is safest.

Jeremy.
Comment 12 Karolin Seeger 2010-03-02 06:41:48 UTC
Pushed both patches to v3-5-test and verified that Metze's patch has been pushed to master.
Closing out bug report.

Thanks!
Comment 13 Stefan Metzmacher 2010-04-06 08:21:58 UTC
Created attachment 5597 [details]
Patch for v3-4
Comment 14 Jeremy Allison 2010-04-07 18:16:04 UTC
Lookd good to me. Re-assigning to Karolin for inclusion in 3.4.x.
Jeremy.
Comment 15 Karolin Seeger 2010-04-08 05:04:21 UTC
Pushed to v3-4-test.
Closing out bug report.

Thanks!