Bug 8021 - Incorrect string termination in volume/volume_name for TRANS2-QUERY_FS_INFO/Info Volume
Incorrect string termination in volume/volume_name for TRANS2-QUERY_FS_INFO/I...
Status: ASSIGNED
Product: Samba 3.5
Classification: Unclassified
Component: File services
3.5.6
All All
: P5 minor
: ---
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-17 12:09 UTC by Volodymyr Khomenko
Modified: 2012-05-30 19:01 UTC (History)
3 users (show)

See Also:


Attachments
Problematic reply: volume lenth is 2 bytes longer due zero-char termination (124.83 KB, image/png)
2011-03-17 12:09 UTC, Volodymyr Khomenko
no flags Details
The same scenario on WinXP: no zero termination (125.33 KB, image/png)
2011-03-17 12:11 UTC, Volodymyr Khomenko
no flags Details
git-am fix for 3.5.next (938 bytes, patch)
2011-03-24 21:26 UTC, Jeremy Allison
metze: review-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Volodymyr Khomenko 2011-03-17 12:09:34 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 [
Unknown error/failure
]

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:

--- a/source3/smbd/trans2.c
+++ b/source3/smbd/trans2.c
@@ -2979,7 +2979,7 @@ cBytesSector=%u, cUnitTotal=%u, cUnitAva
                                pdata, flags2,
                                pdata+l2_vol_szVolLabel, vname,
                                PTR_DIFF(end_data, pdata+l2_vol_szVolLabel),
-                               STR_NOALIGN|STR_TERMINATE);
+                               STR_NOALIGN);
                        SCVAL(pdata,l2_vol_cch,len);
                        data_len = l2_vol_szVolLabel + len;
                        DEBUG(5,("smbd_do_qfsinfo : time = %x, namelen = %d, name = %s\n",

Thanks.
Comment 1 Volodymyr Khomenko 2011-03-17 12:11:01 UTC
Created attachment 6300 [details]
The same scenario on WinXP: no zero termination
Comment 2 Jeremy Allison 2011-03-18 00:21:17 UTC
This looks correct to me. I'll put into master and v3-6-test and create a fix for 3.5.next.
Thanks !
Jeremy.
Comment 3 Jeremy Allison 2011-03-24 21:26:19 UTC
Created attachment 6344 [details]
git-am fix for 3.5.next
Comment 4 Jeremy Allison 2011-03-24 21:26:58 UTC
Re-assigning to Karolin for inclusion in 3.5.next.
Jeremy.
Comment 5 Karolin Seeger 2011-03-25 19:46:04 UTC
(In reply to comment #4)
> Re-assigning to Karolin for inclusion in 3.5.next.
> Jeremy.

Sorry, need a second reviewer before pushing...
Comment 6 Stefan Metzmacher 2011-03-25 21:17:19 UTC
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.
»·······»·······»······· */
Comment 7 Stefan Metzmacher 2011-03-25 21:19:30 UTC
Jeremy please also remove the comment. Testing with OS/2 would also be good.

metze
Comment 8 Jeremy Allison 2011-03-25 21:34:18 UTC
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.

Thanks !

Jeremy.
Comment 9 Stefan Metzmacher 2012-05-30 19:01:07 UTC
Jeremy, what should we do with this bug?