9669 Extra zeros sent by sendfile()

Review Request #1141 — Created July 24, 2018 and submitted

Information
marcel
illumos-gate
master
9669
451cf09...
Reviewers
general 
This fixes a bug in the sendfile(3ext)/sendfilev(3ext) implementation where
some extra zeros could be written to output in a case we asked to copy more
data than there are in the input file.
Without the fix
---------------

# dtrace -n 'tcp_zcopy_check:return /pid==$target/ {trace(arg1)} snf_segmap:entry /pid==$target/ {stack();trace(arg3)}' -c ./test-64
dtrace: description 'tcp_zcopy_check:return ' matched 2 probes
sendfile call: off = 0, len = 8192
sendfile: ret = 8192, off = 8192, errno = 0
sendfile call: off = 20, len = 8192
sendfile: ret = 8192, off = 8212, errno = 0
dtrace: pid 675 has exited
CPU     ID                    FUNCTION:NAME
  0  57683           tcp_zcopy_check:return                 1
  0  59899                 snf_segmap:entry
              genunix`sendvec_chunk+0xb94
              genunix`sendfilev+0x36d
              unix`sys_syscall+0x177
             8192
  0  59899                 snf_segmap:entry
              genunix`sendvec_chunk+0xb94
              genunix`sendfilev+0x36d
              unix`sys_syscall+0x177
             8192

#


With the fix
------------

# dtrace -n 'tcp_zcopy_check:return /pid==$target/ {trace(arg1)} snf_segmap:entry /pid==$target/ {stack();trace(arg3)}' -c ./test-64
dtrace: description 'tcp_zcopy_check:return ' matched 2 probes
sendfile call: off = 0, len = 8192
sendfile: ret = 6, off = 6, errno = 0
sendfile call: off = 20, len = 8192
sendfile: ret = 0, off = 20, errno = 0
dtrace: pid 100698 has exited
CPU     ID                    FUNCTION:NAME
  0  49879           tcp_zcopy_check:return                 1
  0  52107                 snf_segmap:entry
              genunix`sendvec_chunk+0xbe3
              genunix`sendfilev+0x3ba
              unix`sys_syscall+0x27d
                6

#
  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
tsoome
tsoome 
  1. could you also post the test-64 source in testing done or is it part of some test suite already?
    1. https://www.illumos.org/issues/9669#note-1
  2. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/uts/common/syscall/sendfile.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Is there a reason that we're performing this check outside of snf_segmap? Isn't this something that all callers of snf_segmap need to do and therefore we should not be sending data beyond that which we can read?

Given that we have checks in snf_segmap that seem to adjust the total size, is there a reason that we're not leveraging those and cleaning them up versus asking for a different length at this point?

Does sosendfile64 suffer from the same problem as it also calls snf_segmap?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Callers of snf_segmap() (similarly as callers of snf_cache()) are expected to do the check before these functions are called.  Similarly as it is done in sosendfile64().  Yes, I noticed that we could leverage similar checks in both snf_segmap() and snf_cache() just by moving them at the begining of the loop (in both functions), but there would be a problem in sosendfile64() where we need to know the real size to transfer before the snf_segmap() is called, because based on the size we decide to call either snf_segmap() or snf_cache().  So I decided it is far easier just to follow the current habit and adjust the length before the snf_segmap() is called.
      

      No, sosendfile64() does not suffer from the same problem as it does similar size check (and adjust) around line 2900 in socksyscalls.c.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      OK, that's reasoanble. Would you mind adding a comment to that effect to snf_segmap that we need to have adjusted the total size based on the vnode's size?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Yes, this makes sense.  Done.
      
 
      

      4. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




marcel




   

   

  

 



 















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




marcel




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
commit 8cd3131235b232e4d63be3cf95ce9be87907e74f
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Wed Jul 25 12:06:18 2018 +0200
Commit:     Richard Lowe <richlowe@richlowe.net>
CommitDate: Mon Aug 6 19:38:36 2018 +0000

    9669 Extra zeros sent by sendfile()
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

:100644 100644 21f3744895 4cbd079539 M	usr/src/uts/common/fs/sockfs/socksyscalls.c
:100644 100644 822f14416c 0cfafbf13f M	usr/src/uts/common/syscall/sendfile.c





 


 










  

 









     

    

   

  

  

   
   Loading...