Yeah, there are a few more things that look very sketchy. Let me take another
look...
On Oct 30, 2012, at 3:04 PM, Nicolas Burtnyk wrote:
> Hi Larry,
>
> I took a peek at your pull request and although it looks like this
> change will cleanly close tga files on write errors, it still seems
> like we're bailing too early in the case of empty author or document
> name fields.
>
> For example, in the block of code below, if the image spec doesn't
> have an "Artist" attribute (and hence tmpstr is empty), we early
> return false form write_tga20_data_fields skipping all other fields.
>
> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
> return false;
>
> Is that the intended behavior?
>
> I guess the core of the problem is that the fwrite helper function is
> returning false not only on write errors, but also when it is asked to
> write zero bytes and no attempt is made to distinguish bewteen those 2
> cases.
>
> Perhaps the solution is to have the helper fwrite return true when
> itemsize or nitems is zero?
>
> Also, I noticed the block of code that fills with zeros after writing
> the "job ID" field (taken from the "DocumentName" attribute) looks
> kind of sketchy:
>
> // job ID
> tmpstr = m_spec.get_string_attribute ("DocumentName", "");
> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
> return false;
>
> // fill the rest with zeros
> for (int i = 41 - std::min (tmpstr.length (), size_t(40)); i > 0; i--) {
> if (!fwrite (&tmpint) ||
> !fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40))))
> return false;
> }
>
> Why is it trying to write the DocumentName string again every time it
> successfully writes 1 zero?
>
>
> -Nicolas
>
>
> On Tue, Oct 30, 2012 at 12:44 PM, Larry Gritz <[email protected]> wrote:
>> https://github.com/OpenImageIO/oiio/pull/448
>>
>>
>> On Oct 30, 2012, at 10:53 AM, Larry Gritz wrote:
>>
>>> Thanks for pointing that out.
>>>
>>> Yes, in fact there are also several places in TGAOutput::open() where the
>>> file handle is not properly closed.
>>>
>>> Sounds easy to fix. Stay tuned for a pull request from me.
>>>
>>>
>>>
>>> On Oct 30, 2012, at 10:39 AM, Nicolas Burtnyk wrote:
>>>
>>>> While testing OIIO, I noticed that TGAs that were being written to
>>>> disk were not being closed despite calling ImageOutput::close().
>>>> Indeed fclose is not called in TGAOutput::close when the "Artist"
>>>> attribute is empty.
>>>> The problematic block of code is:
>>>>
>>>> if (!fwrite (tmpstr.c_str(), std::min (tmpstr.length (), size_t(40)))) {
>>>> return false;
>>>> }
>>>>
>>>> Since tmpstr is empty, the itemsize parameter of fwrite is 0 and
>>>> fwrite returns false, which in turn makes TGAOutput::close early
>>>> return false (skipping the call to fclose).
>>>>
>>>> Writing of other string attributes are similarly problematic when the
>>>> attribute is empty.
>>>>
>>>> -Nicolas
>>
>> --
>> Larry Gritz
>> [email protected]
>>
>>
>> _______________________________________________
>> Oiio-dev mailing list
>> [email protected]
>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
> _______________________________________________
> Oiio-dev mailing list
> [email protected]
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
