Samba4 illegal dlists and crashing for and-x chaining around alpha-17

[Sam Liddicott]

I think I have found an illegal DLIST problem with the chain handling in
Samba4 around alpha 17 which can lead to segfaults and silent corruptions.
The problem probably exists in most previous release. I know request
handling is reworked recently and I don't know if this bug still exists in
the same or some other form.

The example case covers SMBntcreateX chained with SMBreadX as issued by a
Mac Lion client on temporary files like .DS_Store

When each chained request part is processed, the same smbsrv_request
instance is used (fair enough), however the macro SMBSRV_CALL_NTVFS_BACKEND
is called in the smbsrv_reply_* functions will add the smbsrv_request to
the list req->smb_conn->requests.

smbsrv_reply_ntcreate_and_X invokes SMBSRV_CALL_NTVFS_BACKEND which adds
req to the list.

smbsrv_reply_read_and_X function is later called, req is already part of
the list and is added again by SMBSRV_CALL_NTVFS_BACKEND creating an
illegal list.

The illegal nature of the list depends on whether or not the req was the
last member of the list when it was re-added

When the req is trasmitted and freed it will remove itself from the list
ONCE and can thus leave a bogus entry in the list whose memory is freed and
might be re-allocated. As new items are added, the prev/next fields of this
now-free memory will be amended, and so this can cause segfaults or other
silent corruptions when other items are added to the list.

The errors become more apparent (causing infinite lists) when DLIST_ADD_END
was replaced with DLIST_ADD during debugging.

The quick fix is for SMBSRV_CALL_NTVFS_BACKEND to check if req is already a
member of the list before adding it. The simplest method to do this is to
check that ->prev and ->next are both null -- as we don't expect a single
req to belong to more than one list in its lifetime. This is possible
because smbsrv_init_request uses talloc_zero to allocate the smbsrv_request

The nature of the corruption and of the illegal list IS timing sensitive
and depends on what other requests are processed between the different
chained parts being processed. A synchronous server would add req to the
end of the list where the final list member is the same req. This may be
harmless, my head hurts too much to check.

The patch would be something like the one attached.

Sam Liddicott
-------------- next part --------------
A non-text attachment was scrubbed...
Name: chain-list.patch
Type: text/x-patch
Size: 652 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120309/f4f2d4ab/attachment.bin>