Bug 8806 - illegal dlists and crashing for and-x chaining
Summary: illegal dlists and crashing for and-x chaining
Alias: None
Product: Samba 4.0
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.0 alpha 17
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Tridgell
QA Contact: samba4-qa@samba.org
Depends on:
Reported: 2012-03-10 08:40 UTC by Samjam
Modified: 2012-04-03 13:20 UTC (History)
1 user (show)

See Also:

Patch (653 bytes, patch)
2012-03-10 08:40 UTC, Samjam
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Samjam 2012-03-10 08:40:13 UTC
Created attachment 7368 [details]

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.
Comment 1 Matthias Dieter Wallnöfer 2012-03-11 09:09:01 UTC
Jelmer, is this patch correct? Or better reassign this to Andrew?
Comment 2 Stefan Metzmacher 2012-04-03 12:10:10 UTC
I think it should be fixed with
Comment 3 Samjam 2012-04-03 12:44:36 UTC
For reasons style, I prefer Metze's patch.