Bug 7399 - SMB2: QUERY_DIRECTORY is returning invalid values.
SMB2: QUERY_DIRECTORY is returning invalid values.
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: SMB2
unspecified
x86 Solaris
: P3 major
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-29 10:25 UTC by Ira Cooper
Modified: 2010-06-14 11:45 UTC (History)
0 users

See Also:


Attachments
Proposed patch to fix the issue. (1.05 KB, application/octet-stream)
2010-04-29 10:27 UTC, Ira Cooper
no flags Details
Update of your patch. (4.06 KB, patch)
2010-04-29 14:56 UTC, Jeremy Allison
no flags Details
The program we are using to reproduce things. (76.95 KB, application/x-zip-compressed)
2010-04-30 09:38 UTC, Ira Cooper
no flags Details
Linux code to create X random names. (730 bytes, patch)
2010-04-30 13:32 UTC, Jeremy Allison
no flags Details
tar file of the directory I'm testing with. (785.67 KB, application/octet-stream)
2010-04-30 15:56 UTC, Jeremy Allison
no flags Details
Pretty sure this will nail it (in conjunction with the earlier patch). (654 bytes, patch)
2010-05-13 17:51 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ira Cooper 2010-04-29 10:25:42 UTC
Fix for QUERY_DIRECTORY returning too large a value.

    QUERY_DIRECTORY was returning incorrect values because it was not taking the
    8 byte header for the response into account when calculating how large a
    response it could make.

Patch included.

-Ira
Comment 1 Ira Cooper 2010-04-29 10:27:50 UTC
Created attachment 5671 [details]
Proposed patch to fix the issue.

I am a bit unhappy with this patch.  I don't like that if I move where the -8 is taken into account the patch stops working.  It should be able to be moved to smbd_smb2_request_process_find, but if I move it there, the patch stops working.  There may be a deeper issue.
Comment 2 Jeremy Allison 2010-04-29 12:07:32 UTC
Can yuo give me a quick hint on exactly how you reproduced this ? I agree with you about where the -8 should be - I'll look into the buffer calculations.

Thanks.

Jeremy
Comment 3 Ira Cooper 2010-04-29 13:06:08 UTC
(In reply to comment #2)

Large listings, and then actually checking if the files exist.

~1700 files with decent sized filenames were used I believe.

-Ira
Comment 4 Jeremy Allison 2010-04-29 14:56:05 UTC
Created attachment 5673 [details]
Update of your patch.

Ok, so here is the patch I think works. It moves the -8 into the _send function. I've tested this in a directory with 10,000 files and all of them are seen correctly on a Win7 box.
Can you confirm it's correct in your test setup please ?

Jeremy.
Comment 5 Jeremy Allison 2010-04-29 15:00:25 UTC
Sorry, I meant the patch moves the -8 into the smbd_smb2_request_process_find(), function not the _send function.

Jeremy.
Comment 6 Ira Cooper 2010-04-30 06:38:38 UTC
The proposed change fails my local checks, alas.

In my experimentation, it looked like the exact spot I put that -8 was important.  I wanted to put it where you did, but things went wrong when I did.  The problem may be more "subtle" than my diagnosis.
Comment 7 Ira Cooper 2010-04-30 06:40:50 UTC
(In reply to comment #6)

I'll also see if I can get you a test program etc.

(I need to talk to the author of the test, and get a version to submit.)

Hopefully with that, you can reproduce this at will.
Comment 8 Ira Cooper 2010-04-30 09:38:26 UTC
Created attachment 5676 [details]
The program we are using to reproduce things.

Full source included.  

Invoke as:

probefilelimits.exe -v -c –w <some path with many files named in varying lengths >

We've been using UNC paths, but that doesn't mean you have to.
Comment 9 Jeremy Allison 2010-04-30 11:47:53 UTC
Ok, when I try this program in my 10,000 file directory, it works perfectly.

Ergo there's something about the file names you generate that cause this problem to occur. What do your names look like ? Knowing this would certainly help.

Can you give me a tar file containing just the directory names you're testing against ? I don't need the contents I wouldn't think.

Jeremy.
Comment 10 Jeremy Allison 2010-04-30 13:32:43 UTC
Created attachment 5677 [details]
Linux code to create X random names.

Ok, here is a junkcode program I wrote to create N random length filenames in a directory. Even using this I can't reproduce your issue with current master.

Invoke using:

./mkbigvardir <count> <rand-seed>

and it will create count-1 files in a directory, with names file.<random lower case letters>.

If you can get me a tar list of the directory you use that creates the problem I'd appreciate it - otherwise I'm stuck here as I just can't reproduce.

Jeremy.
Comment 11 Ira Cooper 2010-04-30 15:15:04 UTC
It's worse than that: With your program it reproduces here.

./rndfile 10000 5

So, sending you a tar won't help much.  By chance are you on a 32 or 64 bit platform, and which are you building?  (I build full 64 bit... so I wonder if that's the issue.)

-Ira
Comment 12 Jeremy Allison 2010-04-30 15:42:15 UTC
No - not reproducing.

With ./rndfile 10000 5 I get a directory containing 9970 files (others must have been duplicate names). Running your program against it completes successfully.

What platform are you running on ? You're Solaris - right ? I'm running on 64-bit Linux with an ext3 filesystem. This is starting to smell like the old *BSD directory bug somehow....

http://www.vnode.ch/fixing_seekdir


Jeremy.

Comment 13 Jeremy Allison 2010-04-30 15:56:50 UTC
Created attachment 5678 [details]
tar file of the directory I'm testing with.

Here is the directory I'm testing with on the server side.
Created via :

./rndfile out 10000 5
Comment 14 Ira Cooper 2010-04-30 16:09:59 UTC
Ok, well... let's step back:

If I put the -8 back to where I originally put it.  It all works.

Did the original bug reproduce for you?

-Ira
Comment 15 Jeremy Allison 2010-04-30 16:45:38 UTC
Hmmm. Ok, this is getting interesting. If I simply *remove* the:

in_output_buffer_length -= 8;

line from my tree and recompile, your program still doesn't fail when tested in
the directory I uploaded as a tar file.

I'm testing on a 64-bit Linux box, with a 32-bit Win7 client. Can you upload
your debug level 10 log from smbd when it fails ? I added several debug
statements in smbd/smb2_find.c and in smbd/trans2.c when I committed the change
into master which should give greater insight into what is going on in your
failure case.

Jeremy.
Comment 16 Jeremy Allison 2010-05-13 17:51:43 UTC
Created attachment 5707 [details]
Pretty sure this will nail it (in conjunction with the earlier patch).
Comment 17 Karolin Seeger 2010-06-11 04:24:47 UTC
Change components
Comment 18 Jeremy Allison 2010-06-14 11:45:28 UTC
Bug fixed in master. Will be fixed for 3.6.0.
Jeremy.