[opensource-dev] Review Request: VWR-25608 error on shutdown due to buffer overrun in LLVFS::audit

[Brad Kittenbrink]

> On April 27, 2011, 3:30 a.m., Boroondas Gupte wrote:
> > indra/llvfs/llvfs.cpp, lines 1716-1717
> > <http://codereview.secondlife.com/r/278/diff/1/?file=1511#file1511line1716>
> >
> >     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?

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.


> On April 27, 2011, 3:30 a.m., Boroondas Gupte wrote:
> > indra/llvfs/llvfs.cpp, lines 1719-1723
> > <http://codereview.secondlife.com/r/278/diff/1/?file=1511#file1511line1719>
> >
> >     e.g. here we could extend the condition to
> >     	if (!buffer.empty() && (fread(&buffer[0], 1, index_size, mIndexFP) != index_size))

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


On April 27, 2011, 3:30 a.m., Brad Kittenbrink wrote:
> > 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?

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.


- Brad


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/278/#review654
-----------------------------------------------------------


On April 26, 2011, 5:31 p.m., Brad Kittenbrink wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/278/
> -----------------------------------------------------------
> 
> (Updated April 26, 2011, 5:31 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Fix for a minor buffer overrun on shutdown in LLVFS::audit.
> 
> 
> This addresses bug VWR-25608.
>     http://jira.secondlife.com/browse/VWR-25608
> 
> 
> Diffs
> -----
> 
>   indra/llvfs/llvfs.cpp UNKNOWN 
> 
> Diff: http://codereview.secondlife.com/r/278/diff
> 
> 
> Testing
> -------
> 
> I tested using the Microsoft Debug Heap and confirmed that this allows the Debug Heap to shut down without errors.
> 
> 
> Thanks,
> 
> Brad
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110427/66c8e76e/attachment.htm