Bug 10559 - Computer manager doesn't show accurate files open/lock information
Summary: Computer manager doesn't show accurate files open/lock information
Status: RESOLVED WORKSFORME
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: 3.6.12
Hardware: All All
: P5 normal
Target Milestone: ---
Assignee: Volker Lendecke
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-21 08:20 UTC by Shilpa Krishnareddy
Modified: 2014-04-23 12:54 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Shilpa Krishnareddy 2014-04-21 08:20:02 UTC
# of files open and file locks are not accurate when using MMC open files and it displays stale information.

It was found that the routine share_mode_forall() which is responsible for smbstatus output to print the locked files information is also the one which is called when we try to view open file information from Windows MMC. There is one difference though. smbstatus filters out UNUSED share mode entries and lists only the valid entries. On the other hand, when we are trying to send information to the MMC open files, we do not filter the UNUSED entries and thus MMC open files can display inaccurate (stale) information.

The fix for this issue is to modified traverse_fn() in locking.c such that we call is_valid_share_mode_entry() before we pass share_mode_entry information to the routine that processes it:

        for (i=0; i<lck->num_share_modes; i++) {
                struct share_mode_entry *se = &lck->share_modes[i];
+               if (!is_valid_share_mode_entry(se)) {
+                        continue;
+                }
                state->fn(se,
                        lck->servicepath,
                        lck->base_name,
Comment 1 Volker Lendecke 2014-04-21 09:44:33 UTC
Would this also work for you if you put the is_valid_share_mode_entry into the callback functions in srv_srvsvc_nt.c? While your patch probably correct, I would have to audit the other callers to see if they need the UNUSED entries. Also, in 4.0 and beyond this should not be an issue anymore, we have removed the UNUSED entries in locking.tdb. Is upgrading an option for you?
Comment 2 Shilpa Krishnareddy 2014-04-22 11:29:21 UTC
traverse_fn() is called only by share_mode_forall(). So, I checked the callers of share_mode_forall():

$ find . -name *.c | xargs grep share_mode_forall
./source3/web/statuspage.c:     share_mode_forall(print_share_mode, NULL);
./source3/rpc_server/srvsvc/srv_srvsvc_nt.c:    share_mode_forall( enum_file_fn, (void *)&f_enum_cnt );
./source3/rpc_server/srvsvc/srv_srvsvc_nt.c:    share_mode_forall( sess_file_fn, &s_file_cnt );
./source3/rpc_server/srvsvc/srv_srvsvc_nt.c:    share_mode_forall(enum_file_close_fn, &state);
./source3/utils/status.c:               result = share_mode_forall(print_share_mode, NULL);
./source3/locking/locking.c:int share_mode_forall(void (*fn)(const struct share_mode_entry *, const char *,


In statuspage.c and status.c, we already call is_valid_share_mode_entry() in the callback routine print_share_mode() to eliminate UNUSED entries. If we follow the approach of print_share_mode() routine, then, we need to introduce is_valid_share_mode_entry() in enum_file_fn(). So, I checked the remaining 2 routines sess_file_fn() and enum_file_close_fn() and looks like we need not pass UNUSED entries to these 2 routines. So, I made the changes in traverse_fn() so as to make the fix generic.

Based on the above info, please let me know if the proposed fix is fine. Otherwise, as you suggested, we can make the changes in srv_srvsvc_nt.c. 

As for upgrade, currently, it is not feasible due to some reasons.
Comment 3 Volker Lendecke 2014-04-22 11:58:55 UTC
Ok, that's exactly the audit of the functions hesitated to do. Based on your comment, sure the patch is perfectly fine. 

The next question now is more from a release management point of view:

https://wiki.samba.org/index.php/Samba_Release_Planning

says that Samba 3.6 is in security fixes only mode. So we would have to argue that this is a security issue to accept upstream.

If however this or a similar issue appears in 4.0 or 4.1, we would be happy to put the patch into the next 4.0.x and 4.1.x releases.
Comment 4 Shilpa Krishnareddy 2014-04-23 11:10:23 UTC
I think I cannot claim that the issue can cause any security hole...
Comment 5 Volker Lendecke 2014-04-23 12:54:10 UTC
Ok, with this comment I think it's fair to close this as WORKSFORME. This does not mean your patch is bad, it's just that we won't put it back into 3.6 and I believe it's no longer an issue in 4.0 and beyond. If you ever upgrade and find a similar issue in a version of Samba that is still seeing normal upstream bug fix releases, please re-open this bug or open a new one.