All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

VWR-25608 error on shutdown due to buffer overrun in LLVFS::audit

Review Request #278 - Created April 26, 2011 and submitted

Brad Kittenbrink Reviewers
http://bitbucket.org/lindenlab/viewer-development viewer
VWR-25608
None viewer-development 
Fix for a minor buffer overrun on shutdown in LLVFS::audit.
I tested using the Microsoft Debug Heap and confirmed that this allows the Debug Heap to shut down without errors.
Ship it!
Stone Linden
Posted (April 26, 2011, 5:48 p.m.)
hg rm llconfusingcode.cpp is better, but llmax() works here.
Boroondas Gupte
Posted (April 27, 2011, 3:30 a.m.)

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/llvfs/llvfs.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


       
      

     

    

    

     
When index_size == 0, wouldn't it be more appropriate to skip the steps that require taking the address of element 0? They'd be nil operations in that case, anyway, wouldn't they?


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    In theory, you're right, but my intent was to make the minimally invasive change that would fix the error, and be sure to preserve all existing behaviors.  I was hesitant to engage in any more extensive refactoring without a good plan for how we want to test this subsystem.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Fair enough.
    
    
    
   
    3. 

   


  





    

    

    

     
     

      

       
        

         
          indra/llvfs/llvfs.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
e.g. here we could extend the condition to
	if (!buffer.empty() && (fread(&buffer[0], 1, index_size, mIndexFP) != index_size))


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    We certainly could, but I'm not convinced that that's better.
    
    
    
   
    2. 

   


  





    

    

    

     
     

      

       
        

         
          indra/llvfs/llvfs.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


       
      

     

    

    

     
If index_size == 0, we don't even enter this loop ...


     

  

   

   
    
    
   


  





    

    

    

     
     

      

       
        

         
          indra/llvfs/llvfs.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


        
 


        
 


        
 


       
      

     

    

    

     
... so taking the address here shouldn't be problematic.


     

  

   

   
    
    
   


  





    

    

   


  
   
Finally, if we are only ever accessing the underlying memory directly (as seems to be the case here), why use a std::vector as buffer instead of an array?

   

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I can't speak to the intent of the original author of this code, but I always use vectors for dynamically sized arrays, as they automatically free the buffer.  Using new[] or malloc is far more error prone, even when using std::auto_ptr or boost::scoped_ptr for RAII.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Ah, right ... I keep forgetting that constant sized isn't enough for being statically sized.
    
    
    
   
    3. 

   


  





  
 




  

 



















 

  




 

 

  
Ship it!

  

  
Boroondas Gupte

  
Posted  (April 27, 2011, 12:40 p.m.)

 

 

   
Ready to rock!