[opensource-dev] Review Request: STORM-1118 Viewer crashes when user tries to upload image without JFIF header

Vadim ProductEngine vsavchuk at productengine.com
Mon Apr 11 16:31:54 PDT 2011



> On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote:
> > indra/llimage/llimagedimensionsinfo.cpp, line 97
> > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line97>
> >
> >     Magic number. (I guess it's BMP header (14) + DIB header - current position (File begin + 2)?)

Yep, pretty obvious. If I start documenting every line, code will eventually look even worse than without comments.


> On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote:
> > indra/llimage/llimagedimensionsinfo.cpp, line 147
> > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line147>
> >
> >     ... until here. (Assuming it's the same 8.) Might be worth another constant.

Come on, Boroondas, it wasn't difficult to find! ;-)


> On April 8, 2011, 4:51 p.m., Boroondas Gupte wrote:
> > indra/llimage/llimagedimensionsinfo.cpp, lines 217-219
> > <http://codereview.secondlife.com/r/255/diff/2/?file=1426#file1426line217>
> >
> >     Wouldn't a seek be a "cheaper" way to determine size than reading into an actual buffer? (According to indra/llcommon/llapr.h, it returns -1 on failure.)
> >     
> >     There's also a static method LLAPRFile::size, but that seems to operate on not-yet-opened files given by filename.

seek() may go beyond the file end, so we can't use it to check whether the file contains the needed header.
Well, we could seek to the end of file, but accessing the whole file just to find out that it's of wrong type looks like overkill.
I agree that reading into a dynamically allocated buffer doesn't look nice, but I think we can live with it as long as we only read small chunks this way.


- Vadim


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


On April 7, 2011, 4:20 p.m., Vadim ProductEngine wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/255/
> -----------------------------------------------------------
> 
> (Updated April 7, 2011, 4:20 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> * Added checks for image file contents not matching the file extension (e.g. a bitmap named file.jpg).
> * Added checks for abnormally short files to avoid crashes when parsing them.
> 
> 
> This addresses bug STORM-1118.
>     http://jira.secondlife.com/browse/STORM-1118
> 
> 
> Diffs
> -----
> 
>   indra/llimage/llimagedimensionsinfo.h 33ca961b0870 
>   indra/llimage/llimagedimensionsinfo.cpp 33ca961b0870 
> 
> Diff: http://codereview.secondlife.com/r/255/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Vadim
> 
>

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


More information about the opensource-dev mailing list