Terminate empty ASCII strings

[kmilos]

Fixes #1484

[clanmills]

Can you augment the test suite, please?

Is there a less obtuse if statement, such as:

value_ += value_.size() ? '' : '\0';

[kmilos]

Not the same thing - I presume the check whether the non-empty string is already terminated was there for a reason...

I'll work on that test case.

[clanmills]

If you don't want to change that code, please a comment to explain it and reference this bug report.

I would prefer that you change it.

[clanmills]

I've changed my mind. Your code's good and I'm happy if you add a comment.

 int AsciiValue::read(const std::string& buf)
    {
        value_ = buf;
        // ensure count>0 and nul terminated # https://github.com/Exiv2/exiv2/issues/1484
        if (value_.size() == 0 || value_[value_.size()-1] != '\0') value_ += '\0';
        return 0;
    }

Here's a test: tests/bugfixes/github/test_issue_1484.py

# -*- coding: utf-8 -*-

from system_tests import CaseMeta, CopyTmpFiles, path
@CopyTmpFiles("$data_path/Stonehenge.exv")

class test_issue_1484(metaclass=CaseMeta):
    url       = "https://github.com/Exiv2/exiv2/issues/1484"
    filename  = path("$tmp_path/Stonehenge.exv")
    commands  = [ "$exiv2 -g Copyright                            $filename" 
                , "$exiv2 -M\"set Exif.Image.Copyright Ascii ''\" $filename"
                , "$exiv2 -g Copyright                            $filename"
                ]
    stdout   =  ["","","""Exif.Image.Copyright                         Ascii       1  
"""]
    stderr = [""]*len(commands)
    retval = [ 1,0,0]

[kmilos]

Thanks Robin, you were quicker ;)

[clanmills]

We want to do things quickly and efficiently. When the CI turns green, I will merge and close this one.

Much the same with the mime matters. If you tweak BmffImage::mimeType() to generate the mime types that you consider appropriate, I'll update the test code.

[clanmills]

Good Job. Thanks for doing this.

[clanmills]

Whoops. Found a way to break the fix! If you fix the C++, I'll give you the modified code for test_issue_1484.py

845 rmills@rmillsm1:~/temp $ exiv2 -M'set Exif.Image.Copyright ' Stonehenge.jpg 
846 rmills@rmillsm1:~/temp $ exiv2 -g Copyright Stonehenge.jpg 
Exif.Image.Copyright                         Ascii       0  
847 rmills@rmillsm1:~/temp $ 

[kmilos]

Nice catch, grrrr...

The debugger session will have to wait for a slot.

[clanmills]

The revised code is:

// ensure count>0 and nul terminated # https://github.com/Exiv2/exiv2/issues/1484
if (value_.size() == 0 || (value_.size() > 0 && value_[value_.size()-1] != '\0')) value_ += '\0';

And the test is:

# -*- coding: utf-8 -*-

from system_tests import CaseMeta, CopyTmpFiles, path
@CopyTmpFiles("$data_path/Stonehenge.exv")

class test_issue_1484(metaclass=CaseMeta):
    url       = "https://github.com/Exiv2/exiv2/issues/1484"
    filename  = path("$tmp_path/Stonehenge.exv")
    commands  = [ "$exiv2 -g Copyright                            $filename" 
                , "$exiv2 -M\"set Exif.Image.Copyright Ascii ''\" $filename"
                , "$exiv2 -g Copyright                            $filename"
                , "$exiv2 -M\"del Exif.Image.Copyright Ascii\"    $filename"
                , "$exiv2 -g Copyright                            $filename"
                , "$exiv2 -M\"set Exif.Image.Copyright\"          $filename"
                , "$exiv2 -g Copyright                            $filename"
                , "$exiv2 -M\"set Exif.Image.Copyright me 2021-\" $filename"
                , "$exiv2 -g Copyright                            $filename"
                ]
    stdout   =  ["","",
"""Exif.Image.Copyright                         Ascii       1  
""","","","",
"""Exif.Image.Copyright                         Ascii       1  
""","",
"""Exif.Image.Copyright                         Ascii       9  me 2021-
"""]
    stderr = [""]*len(commands)
    retval = [1,0,0,0,1,0,0,0,0]
    

[clanmills]

@kmilos. I've pushed the fix. If you're happy and the CI is green, please merge and close.