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

[Sam Liddicott]

Nudge.

This seems simple enough and I don't want it to get forgotten.

https://bugzilla.samba.org/show_bug.cgi?id=8806

Sam

On Fri, Mar 9, 2012 at 3:14 PM, Sam Liddicott <sam at liddicott.com> wrote:

> 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
>