Deallocated memory access in UpdateDialog::ShowReleaseNotes

[mpleis]

When the size of our appcast description grows too big the IHTMLDocument2 write crashes (AV) in UpdateDialog::ShowReleaseNotes. A sample of the 'large' Description information is attached. The wxBasicString(wxString::FromUTF8(info.Description.c_str())); results in the first four bytes of the BSTR corrupted (looks like a MyVal2 memory address); see image. I used this statement around line 1080 in ui.cpp:

BSTR bar = wxBasicString(wxString::FromUTF8(info.Description.c_str()));

Using a different ctor cleaned up the AV

wxBasicString basicstr = wxBasicString();
basicstr.AssignFromString(wxString::FromUTF8(info.Description.c_str()));

param->bstrVal = basicstr; 

GitIssue.txt

[vslavik]

Can you please provide actual reproduction steps — i.e. a patch to reproduce in the included example app or, if sufficient, at least the appcast feed's URL?

Please also provide basic information, such as your Windows version, locale etc. and your WinSparkle version (and if the answer to the latter is not "the latest master as of today", try with that first).

Using a different ctor cleaned up the AV

Unless you have a rational explanation for that, I'm afraid you're jumping to conclusions here: if you look at the code, both are strictly equivalent. Looks like it's just random shuffling around that happens to hide, not fix, the apparent memory corruption, in this specific instance.

[mpleis]

Windows 10, VS2015, English, I assumed you could bolt in the text attachments into one of your, no doubt, many feeds. I forked and added a Pass (works https://raw.githubusercontent.com/mpleis/winsparkle/master/works_appcast.xml) and Fail (fails https://raw.githubusercontent.com/mpleis/winsparkle/master/fails_appcast.xml) appcast to my fork. Looks like total number of printable characters (white space add/remove did not make any diff) is important to this condition. We used CDATA and element with no pass/fail difference.

At this point I can't accept the constructors are 'strictly equivalent' without ramping up on my C++ and proving to myself your assertion.

No doubt shuffling could be a factor.

Yes, git clone within the last few days.

[mpleis]

I'm not sure if there is much interest in this issue. BSTRs are in a realm of there own WRT scope/ownership. This nugget in ui.cpp seems to behave nicely with the failure case (no leaks). psaStrings = SafeArrayCreateVector(VT_VARIANT, 0, 1); if (psaStrings != NULL) { VARIANT param; SafeArrayAccessData(psaStrings, (LPVOID) &param); param->vt = VT_BSTR;

    wxBasicString basicstr = wxBasicString();  // make local use of OLE conduit
    basicstr.AssignFromString(wxString::FromUTF8(info.Description.c_str()));
    param->bstrVal = basicstr; 

  //    ORIGNAL      param->bstrVal = wxBasicString(wxString::FromUTF8(info.Description.c_str()));
            SafeArrayUnaccessData(psaStrings);

            doc->write(psaStrings); // Blows up
            doc->close();

    SafeArrayDestroy(psaStrings);
    basicstr.Detach();  // pitch local use of BSTR
        }

[vslavik]

I'm not sure if there is much interest in this issue.

You are being inappropriate. You are using this open source project commercially, but don’t pay for commercial support; you can’t expect support as you seem to. You’re bugging volunteers to deal with something you care about in their free time, instead of doing their jobs or being with their families. It’s been six days since you provided information. I’m terribly sorry that I had to actually, you know, work in those 6 days and couldn’t attend to you.

Have some patience. Help out — make good reports, reduce friction (e.g. provide patches instead of expecting the maintainer to spend additional time following your manual instructions — doing so also verifies it’s not your code and proves that to the maintainer, which speeds up fixing it). Don’t write confusing code snippets — submit a pull request instead. Better yet: debug, investigate, understand the bug so that it can be fixed (shotgun debugging and magical reasoning don’t help with a fix; understanding the cause of behavior does). Or if you’re unwilling to put a bit of work into the issue, wait a bit. I’ll get around to it.

[mpleis]

I don't mean to be impatient. Really appreciate your open source project and I'm trying to contribute. But, GitHub is new to me and I need to learn how to use it better.

[vslavik]

To explain what's happening: changing this:

param->bstrVal = wxBasicString(wxString::FromUTF8(info.Description.c_str()));

to use AssignFromString makes no difference (again, strictly equivalent to the ctor). What does make the difference is that you also changed the lifetime of the wxBasicString instance, so this is the minimal part that makes a difference in your observation:

wxBasicString x(wxString::FromUTF8(info.Description.c_str()));
param->bstrVal = x;

The bug was that the string was deallocated before used, because wxBasicString's implicit conversion doesn't make a copy; in the modified (but still wrong) 2nd snippet it has larger lifetime and so this won't occur.

The proper fix then is to properly transfer ownership of the OLE string during assignment, as ea46234a does.