Created attachment 7368 [details] Patch 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.
Jelmer, is this patch correct? Or better reassign this to Andrew?
I think it should be fixed with https://gitweb.samba.org/?p=samba.git;a=commitdiff;h=6865241fdde71c5f7bbe85b3b
For reasons style, I prefer Metze's patch.