Bug 13578 - Daemozined applications hang when executed within a subshell
Summary: Daemozined applications hang when executed within a subshell
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.9.0rc3
Hardware: x64 Linux
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-16 22:32 UTC by Paulo Alcantara
Modified: 2018-09-06 07:07 UTC (History)
3 users (show)

See Also:


Attachments
Proposed fix (2.23 KB, patch)
2018-08-17 14:59 UTC, Paulo Alcantara
jmcd: review+
jra: review+
Details
git-am fix for 4.9.next, 4.8.next, 4.7.next (2.59 KB, patch)
2018-08-23 22:35 UTC, Jeremy Allison
jmcd: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo Alcantara 2018-08-16 22:32:39 UTC
In case we don't have either a /var/log/samba directory, or pass a                                                                                   
non-existent log directory through '-l' option, all commands that are                                                                                
daemonized with '-D' option hang when executed within a subshell.                                                                                    
                                                                                                                                                     
An example on how to trigger that:                                                                                                                   
                                                                                                                                                     
  # rm -r /var/log/samba                                                                                                                             
  # s=$(nmbd -D -s /etc/samba/smb.conf -l /foo123)                                                                                                   
  (never returns)                                                                                                                                    
                                                                                                                                                     
So, when the above command is executed within a subshell the following                                                                               
happens:                                                                                                                                             
                                                                                                                                                     
  (a) Parent shell creates a pipe, sets write side of it to fd 1 (stdout),                                                                           
    call read() on read-side fd, forks off a new child process and then                                                                              
    executes nmbd in it.                                                                                                                             
  (b) nmbd sets up initial logging to go through fd 1 (stdout) by                                                                                    
    calling setup_logging(..., DEBUG_DEFAULT_STDOUT). 'state.fd' is now                                                                              
    set to 1.                                                                                                                                        
  (c) reopen_logs() is called by the first time which then calls                                                                                     
    reopen_logs_internal()                                                                                                                           
  (d) in reopen_logs_internal(), it attempts to create log.nmbd file in                                                                              
    /foo123 directory and fails because directory doesn't exist.                                                                                     
  (e) Regardless whether the log file was created or not, it calls                                                                                   
    dup2(state.fd, 2) which dups fd 1 into fd 2.                                                                                                     
  (f) At some point, fd 0 and 1 are closed and set to /dev/null                                                                                      
                                                                                                                                                     
The problem with that is because parent shell in (a) is still blocked in                                                                             
read() call and the new write side of the pipe is now fd 2 -- after                                                                                  
dup2() in (e) -- and remains unclosed.                                                                                                               
                                                                                                                                                     
So, IMHO, we should be checking if the log file was created successfuly                                                                              
in (d), and if so, take over stderr with the new created file.                                                                                       
                                                                                                                                                     
Basically, my proposed fix would be replacing the following check in                                                                                 
reopen_logs_internal():                                                                                                                              
                                                                                                                                                     
        /* Take over stderr to catch output into logs */                                                                                             
        if (state.fd > 0) {                                                                                                                          
                if (dup2(state.fd, 2) == -1) {                                                                                                       
                        ...                                                                                                                          
                }                                                                                                                                    
                ...                                                                                                                                  
        }                                                                                                                                            
With:                                                                                                                                                
                                                                                                                                                     
        /* Take over stderr to catch output into logs */                                                                                             
        if (new_fd != -1) {                                                                                                                          
                if (dup2(state.fd, 2) == -1) {                                                                                                       
                        ...                                                                                                                          
                }                                                                                                                                    
                ...                                                                                                                                  
        }                                                                                                                                            
                                                                                                                                                     
By doing so, we properly closes stdout and unblocks read() call from                                                                                 
parent shell and nmbd returns correctly.
Comment 1 Paulo Alcantara 2018-08-16 22:37:51 UTC
An email has been sent to ML[1] along with a proposed fix. I'll send a formal patch as soon as I make sure it passes autobuild / gitlab CI.

Thanks,
Paulo

[1] https://marc.info/?i=87lg9pbs6d.fsf@paulo.ac
Comment 2 Paulo Alcantara 2018-08-17 14:59:34 UTC
Created attachment 14430 [details]
Proposed fix

Follow patch for review & comments. Thanks.
Comment 3 Jeremy Allison 2018-08-17 20:33:12 UTC
Comment on attachment 14430 [details]
Proposed fix

Pushed to master.
Comment 4 Jeremy Allison 2018-08-23 22:35:00 UTC
Created attachment 14445 [details]
git-am fix for 4.9.next, 4.8.next, 4.7.next

Cherry-picked from master.
Comment 5 Jeremy Allison 2018-08-28 16:14:25 UTC
Re-assigning to Karolin for inclusion in 4.9.next, 4.8.next, 4.7.next.
Comment 6 Karolin Seeger 2018-09-04 06:45:19 UTC
(In reply to Jeremy Allison from comment #5)
Pushed to autobuild-v4-{9,8,7}-test.
Comment 7 Karolin Seeger 2018-09-06 07:07:03 UTC
Pushed to all branches.
Closing out bug report.

Thanks!