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
