8098 Some xdr_simple(3nsl) and xdr_complex(3nsl) functions can succeed for undefined xdrs->x_op values

Review Request #442 — Created April 24, 2017 and submitted

Information
marcel
illumos-gate
master
8098
eaab405...
Reviewers
general 
This modifies the following XDR functions from libnsl(3nsl) to fail in a case
an invalid value of xdrs->x_op is passed to them:

xdr_hyper(3nsl)
xdr_opaque(3nsl)
xdr_reference(3nsl)
xdr_vector(3nsl)
I ran the test attached to the bug report:

$ ./test 
xdr_hyper: OK
xdr_opaque: OK
xdr_reference: OK
xdr_vector: OK
xdr_vector: OK
$ LD_PRELOAD=$CODEMGR_WS/proto/root_i386/usr/lib/libnsl.so.1 ./test 
xdr_hyper: FAIL
xdr_opaque: FAIL
xdr_reference: FAIL
xdr_vector: FAIL
xdr_vector: FAIL
$
yuripv
yuripv 
  1. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/lib/libnsl/rpc/xdr.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    May be return TRUE/FALSE here depending on x_op being valid if cnt is 0? This would be an easier to understand logic change.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Yes, I agree that the change would be easier to understand with your suggestion, but from a long term view it will just add some bloatware with no real added value.

The cnt == 0 case is so rare that it does not deserve the special handling here.  Actually, the cnt == 0 case is usually a programming error.  Thus the cnt == 0 check for every xdr_opaque(3nsl) call makes the code very likely a bit slower as it will be with this fix. :-)
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Both suggestions (not marked as "fix it") were just to made xdr_opaque() to looks the same as other 2 functions:
      

      if !valid return false;
if cnt == 0 return true;

      

      

      I'm fine with how it's done currently though if you don't want more involved changes. Ship it!
      
 
      

      3. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/lib/libnsl/rpc/xdr_array.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    Worth creating a macro?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      For just two uses in two different files?  It would need to go to some header file.  Hmmmm, I'd like to avoid headers changes for this fix.  It is not worth doing, I believe.
      
 
      

      2. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 





 

  

   




yuripv




   

   







  

 



 

  


   

   

    
 
yuripv


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




jbk




   

   







  

 



 

  


   

   

    
 
jbk


    
   


  

  

  



    




  1. 
 
    
  
    
   
    It probably would be good to create some XDR tests under usr/src/tests.  Would you be willing to create a followup ticket for that?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry, no.  I do not plan to implement it, so it does not make sense for me to create such a ticket.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




vgusev




   

   







  

 



 

  


   

   

    
 
vgusev


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      BTW, Are you going to create ticket for uts verstion of the xdr functions?
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry, I do not plan to do so now.
      
 
      

      3. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




marcel




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
commit 22cc57556161a28b2141976ff578db2558def3e1
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Sun Apr 23 22:04:04 2017 +0200
Commit:     Robert Mustacchi <rm@joyent.com>
CommitDate: Wed May 3 21:21:34 2017 +0000

    8098 Some xdr_simple(3nsl) and xdr_complex(3nsl) functions can succeed for undefined xdrs->x_op values
    Reviewed by: Yuri Pankov <yuri.pankov@gmail.com>
    Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
    Reviewed by: Vitaliy Gusev <gusev.vitaliy@icloud.com>
    Approved by: Robert Mustacchi <rm@joyent.com>

:100644 100644 6d5095f... dd13140... M	usr/src/lib/libnsl/rpc/xdr.c
:100644 100644 c3b4508... 80e49cf... M	usr/src/lib/libnsl/rpc/xdr_array.c
:100644 100644 e3be8f2... c20ee18... M	usr/src/lib/libnsl/rpc/xdr_refer.c





 


 










  

 









     

    

   

  

  

   
   Loading...