Page MenuHomeFreeBSD

fix (hack) readdir and seekdir to allow samba to correctly delete files
AcceptedPublic

Authored by julian on Apr 30 2015, 4:32 PM.

Details

Reviewers
jhb
rmacklem
glebius
Group Reviewers
manpages
Summary

Samba needs limited seekdir() functionality to work when files have been deleted from the directory during the session. It only needs to be able to back up a single entry when it finds that the directory entries it has already read have filled up its buffer and the next one won't fit. Currently this doesn't work.

Unfortunately the windows client is using these entries to delete files, so when
Samba comes back to read the entry again for the next iteration, the current seekdir() code does the
wrong thing, as the directory has been altered.

This change makes use of the fact that the previous returned item is ALWAYS still in the buffer at the time of the seek back, and this allows us to reliably fetch it again even though the directory has changed.
P.S. This is my first phabricator submission so it may look a bit unpracticed.

Test Plan

I have a test program I will make available separately, and have tested from windows via Samba

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

julian updated this revision to Diff 5114.Apr 30 2015, 4:32 PM
julian retitled this revision from to fix (hack) readdir and seekdir to allow samba to correctly delete files.
julian updated this object.
julian edited the test plan for this revision. (Show Details)
julian added reviewers: jhb, rmacklem, glebius.
emaste added a subscriber: emaste.Apr 30 2015, 4:48 PM
julian updated this object.May 1 2015, 1:43 AM
julian edited edge metadata.
julian updated this object.May 1 2015, 1:48 AM
julian edited the test plan for this revision. (Show Details)
jilles added a subscriber: jilles.May 3 2015, 2:48 PM

Is _fixtelldir() actually needed? I noticed a bug in it, and if loc_loc == 0, there doesn't seem harm in lseek() + getdirentries().

lib/libc/gen/telldir.c
137

This will not work correctly if telldir() did not allocate a new struct ddloc but reused an existing one. One way to fix it is to make telldir() move the reused struct ddloc to the head of the list (but that will also modify the old cookie).

rmacklem accepted this revision.May 5 2015, 10:09 PM
rmacklem edited edge metadata.

If telldir() is called when at the end of a block read by
getdirentries(), loc_seek is set to the block already read
and loc_loc to the end of the block. This is somewhat bogus
and I think that this patch, which replaces loc_seek with the
offset of the next block + sets loc_loc == 0 is an improvement.

I think _fixtelldir() could use a loop to search for a matching
entry instead of just testing the first one, but that would just
help to ensure the replacement occurs whenever possible.

This revision is now accepted and ready to land.May 5 2015, 10:09 PM
julian added a comment.May 7 2015, 2:26 AM

patch committed. (and then some changes added to the comments and man page by jhb)

lib/libc/gen/telldir.c
137

yes you are correct that if the telldir cookie being seeked is not the last that was issued this will not find it, however that was a decision I made in order to keep the patch small.
In the example used by samba the action is always:
telldir();
readdir();
telldir();
readdir();
telldir();
readdir(); // oops it won't fit, put it back..
seekdir();
{work}
telldir();
readdir();

so , as I mentionned this will only allow a reliable backwards step of a single item, and assumes that the telldir we are using is the one just before the read we want to undo.
otherwise it is just as broken as it is now.. I don't pretend to fix any more than that one case. (well actually I accidentally do fix a bit more but that's a by-product.)'
Actually thinking about t a but more, if the "old cookie" in your example was followed by a "readdir" and needed to be fixed the second time, then it would have needed to have been fixed the first time too, so it would already be fixed..
(and as such would not be recognised as a match, so a duplicate would be issued..)
The fix is only ever applied if the previous cookie refers to a position beyond the end of the buffer that has just been completed. Since the data is not IN that old buffer, keeping around such a cookie seems pointless, better to make it point to the real data.

julian added a comment.May 7 2015, 2:59 AM

Committed as @rS282485.

Committed in rS282485, @julian can you close please?