The Samba-Bugzilla – Bug 8021
Incorrect string termination in volume/volume_name for TRANS2-QUERY_FS_INFO/Info Volume
Last modified: 2012-05-30 19:01:07 UTC
Created attachment 6299 [details]
Problematic reply: volume lenth is 2 bytes longer due zero-char termination
Run smbtorture4 RAW-QFSINFO test on samba 3.5.x.
Received following error:
# smbtorture --option=torture:samba3=true //server/vol0 -U Admin%passwd RAW-QFSINFO
check for correct termination
Expected wire_length 8 but got 10 for 'vol0'
(292) incorrect string termination in volume/volume_name
error: QFSINFO [
Reviewed problematic reply with wireshark (see attached screenshot): indeed, in 'QUERY_FS_INFO/Info volume' reply I see that volume name ('vol0' in example) is zero-terminated (while it should be non-terminated). So label length is 2 bytes longer than expected (null char, two bytes per character).
Also I've checked the same flow on Windows XP: the test passes, no zero-char terminator is used (see another attached screenshot - 'sys_vol' label has no zero terminator). So termination should be fixed.
Patch for this case is trivial:
@@ -2979,7 +2979,7 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAva
data_len = l2_vol_szVolLabel + len;
DEBUG(5,("smbd_do_qfsinfo : time = %x, namelen = %d, name = %s\n",
Created attachment 6300 [details]
The same scenario on WinXP: no zero termination
This looks correct to me. I'll put into master and v3-6-test and create a fix for 3.5.next.
Created attachment 6344 [details]
git-am fix for 3.5.next
Re-assigning to Karolin for inclusion in 3.5.next.
(In reply to comment #4)
> Re-assigning to Karolin for inclusion in 3.5.next.
Sorry, need a second reviewer before pushing...
Comment on attachment 6344 [details]
git-am fix for 3.5.next
Jeremy there's a comment above from you:
»·······»·······»······· * Win2k3 and previous mess this up by sending a name length
»·······»·······»······· * one byte short. I believe only older clients (OS/2 Win9x) use
»·······»·······»······· * this call so try fixing this by adding a terminating null to
»·······»·······»······· * the pushed string. The change here was adding the STR_TERMINATE. JRA.
Jeremy please also remove the comment. Testing with OS/2 would also be good.
Oh that's *wonderful* metze. Hoist by my own petard (and also by only testing against Win28kR2 :-).
I'll revert this in master and 3.6.0 until I get to test with OS/2.
Jeremy, what should we do with this bug?