Crash on assigning error_msg to std::string

[smizonov]

In my project i use heif_cxx.h api. And caught crash inside heif_cxx.h.
It happence while assigning err.message to m_message
Error(const heif_error& err) { m_code = err.code; m_subcode = err.subcode; m_message = err.message; }
Attach debug screen with callStack.

I can't show the fields in the structure, because Qt debugger is stupid, but I hope it's clear.
I read in heif.h file that message in heif_error struct always defined, but apparently not always)

[farindk]

Thanks. Can you see the code and subcode in the error message so that I can track down where the missing error text is coming from?

It appears that you are using heif::Context::write() with a Writer class. Could it be that that one returns the error without message?

[smizonov]

code and subcode are 0.
If i comment out this line
m_message = err.message;
Output image stored successfully (after write method i write buffer to file and can open an image).

[farindk]

Thanks, code=subcode=0 means success.

I've searched the whole code where this might be returned with a null text, but could not find anything.
Thus, I'm still wondering whether this is in your code.
When you call Context::Write(), you are passing a writer object. And that should return an heif_error from its write() method. What are you returning there? Do you return an heif_error with a null message? Or maybe you completely forgot the return?

[smizonov]

I am sorry, may be that's my error
Here definition of my writer class

class Writer:
        public heif::Context::Writer
{
public:
    Writer(std::vector<unsigned char> & out):
        outData_(out)
    {};
    heif_error write(const void* data, size_t size) override
    {
        std::copy_n(static_cast<const unsigned char*>(data), size, std::back_inserter(outData_));
        return heif_error{ heif_error_Ok };
    }
private:
    std::vector<unsigned char> & outData_;
};

I think in overrided write function i need fill all fields. I think this may help. Thank you!

[farindk]

Yes, you should return this: heif_error{heif_error_Ok, heif_suberror_Unspecified, "Ok"}.
Can we change the PR such that an assert() is used instead? I think this might be better than to hide a bug elsewhere with the if.
We could also define a default heif_error for the 'ok' value somewhere.

It's a small change, if you want, I can just add this.

[smizonov]

Yes, i can add assert, but as i know assert does not work in release mode. Is it enough that assert will only be in the debug mode?

[farindk]

Yes, that's intended, as software errors should be detected mainly during development.

However, thinking about it again, it might be a better idea to do this

if (!err.message) {
  return Error(heif_error_Usage_error, heif_suberror_Null_pointer_argument, "Writer class returned a null error text");
}

[smizonov]

Hmm, i little confused. You suggest check err.message. But this logic must be in Error constructor. And i cant return Error from Error constructor)
May be your first suggestion will be better.
Add assert to constructor (or throw std::runtime_error).
In heif.h Creating function which would return default error

heif_error default_heif_error()
{
    return heif_error{ heif_error_Ok, heif_suberror_Unspecified, "Ok" };
}

In my case if i saw this function near struct heif_error i will prefer this one.

[smizonov]

Or may be this one?

  Error(const heif_error& err)
    {
      if (!err.msg)
        throw Error(heif_error_Usage_error, heif_suberror_Null_pointer_argument, "Writer class returned a null error text");
      m_code = err.code;
      m_subcode = err.subcode;
      m_message = err.message;
    }

[farindk]

I have made the changes. See the two commits above.
If you also think that they improve the error handling, we can probably close this.

[smizonov]

Got it, thank you. Yes, i think this PR can be closed

[farindk]

Thanks for your help.