View Issue Details

IDProjectCategoryView StatusLast Update
0003332Ham Radio DeluxeBugpublic2019-11-08 02:32
ReporterK7ZCZAssigned ToDOUG 
PrioritynormalSeveritycrashReproducibilityalways
Status closedResolutionfixed 
Product Version6.6.0.214 
Target VersionFixed in Version6.7.0.244 
Summary0003332: QLM license code incorrectly casts string, ignores error return from Soraco function
DescriptionThis code is in LicenseValidator::WriteProductProperties():

string props = license->GetProductProperties(_bstr_t(""), _bstr_t(this->activationKey), &bstrResponse);


This code causes the application to crash because GetProductProperties() might return NULL, and NULL can't be successfully assigned to a std::string.

GetProductProperties() is declared to return a _bstr_t, which means an implicit cast is happening. That cast is dubious, since std::string is intended to represent a plain string (as compared to std::wstring which represents a wide dstring), while _bstr_t objects usually represent wide-character strings.

The correctness of the cast should be reviewed. The code should be fixed so that the error indicated by GetProductProperties() returning NULL is correctly handled and doesn't crash the application.

TagsNo tags attached.
ModuleSW License Mgmt
Sub-ModuleSW License Client
Testing Beta Successful

Relationships

related to 0003396 closedK7ZCZ HRDSoracoLicenser::CheckLicense leaks memory 
related to 0003345 closedDOUG WriteProductProperties method in LicenseValidator catches all exceptions 

Activities

DOUG

2019-06-10 23:33

developer   ~0008036

Cleaned up the old code, not sure why the string was there any way, it was going BSTR->string->char*->then back to BSTR to be used. Looks like it works just fine as BSTR throughout, added the check for null, just returns false in that situation

DOUG

2019-06-11 21:11

developer   ~0008052

Changed to ....

_bstr_t bstrProps = license->GetProductProperties(_bstr_t(""), _bstr_t(this->activationKey), &bstrResponse);

        if (bstrProps.length())
        {


otherwise it returns false.

Does that work for you Mike?

WA9PIE

2019-06-22 11:15

administrator   ~0008165

Is this fixed and checked-in?

K7ZCZ

2019-07-21 06:46

administrator   ~0008254

Looks like this was fixed with this checkin on 2019-06-10:
https://hrdsoftware.visualstudio.com/HRD/_versionControl/changeset/5016

When issues are fixed, we should be marking them "resolved" in Mantis. The checkin should have a comment that clearly indicates which Mantis issue the checkin addresses, and the Mantis issue should get a link to that checkin on the VSTS website.

The code in question was copied-and-pasted to another location. While this issue is fixed, we get to fix the same (or, at least, very similar) problem again in a different place. I've opened 3396 to track that issue.

K7ZCZ

2019-08-15 22:42

administrator   ~0008411

On my laptop (with no QLM developer license), I crash at this calls stack starting Rig Control:

     KernelBase.dll!_RaiseException@16()	Unknown
>	HRDStation.dll!_CxxThrowException(void * pExceptionObject, const _s__ThrowInfo * pThrowInfo) Line 129	C++
     HRDStation.dll!_com_raise_error(HRESULT hr, IErrorInfo * perrinfo) Line 18	C++
     HRDStation.dll!_com_issue_errorex(HRESULT hr, IUnknown * punk, const _GUID & riid) Line 66	C++
     HRDStation.dll!QlmLicenseLib::IQlmLicense::GetProductProperties(_bstr_t webServiceUrl, _bstr_t ActivationKey, wchar_t * * response) Line 3072	C++
     HRDStation.dll!LicenseValidator::WriteProductProperties(ATL::CStringT<wchar_t,StrTraitMFC<wchar_t,ATL::ChTraitsCRT<wchar_t> > > &) Line 784	C++
     HRDStation.dll!LicenseValidator::ValidateLicenseAtStartup(ATL::CStringT<wchar_t,StrTraitMFC<wchar_t,ATL::ChTraitsCRT<wchar_t> > > computerID, bool & needsActivation, ATL::CStringT<wchar_t,StrTraitMFC<wchar_t,ATL::ChTraitsCRT<wchar_t> > > & returnMsg) Line 220	C++
     HRDStation.dll!HRDSoracoLicenser::CheckLicense(int bForceDlg) Line 114	C++
     HRDStation.dll!_HRDInitStation() Line 320	C++
     [Inline Frame] HRDStation.dll!CHRDStationApp::InitializeStation() Line 160	C++
     HRDStation.dll!InitializeStation() Line 129	C++
     HamRadioDeluxe.exe!CHamRadioDeluxeApp::InitInstance() Line 245	C++
     HamRadioDeluxe.exe!AfxWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpCmdLine, int nCmdShow) Line 37	C++
     [Inline Frame] HamRadioDeluxe.exe!invoke_main() Line 118	C++
     HamRadioDeluxe.exe!__scrt_common_main_seh() Line 288	C++
     kernel32.dll!@BaseThreadInitThunk@12()	Unknown
     ntdll.dll!__RtlUserThreadStart()	Unknown
     ntdll.dll!__RtlUserThreadStart@8()	Unknown




Sure, I should have a developer license. I asked for one and for whatever reason none is given.

But in the customer's hands, this same function might make a negative return and we would crash. We have no documentatoin about when it resturns fails or not, so it could happen to any customer at any time. Who knows?

I just ask that we write high-quality, resilient code, which doesn't crash with a negative return value.

K7ZCZ

2019-08-16 12:15

administrator   ~0008413

I dug into this a bit more. Looks like this is a problem in the TLB itself. When the underlying Interface returns NULL, it still returns S_OK. The wrapper code in the TLB tries to construct a _bstr_t from the NULL return value and pitches an exception. The TLB is provided by Soraco, so we'll have to get in touch with them about a fix. And, until then, hope that customers aren't getting NULL values back in the wild.

DOUG

2019-08-16 21:38

developer   ~0008414

I looked into it some last night too.
It definitely is like you said, tied to the dev license and running an api call in the debugger. I should have put 2+2 together earlier. I previously used the qlm admin API for porting the users over, and they had a very nice exception that said something like "Hey you can't run QLM admin api in the debugger"".

I threw process monitor on it, and tracked it down to the registry entry It is validating in the debugger.

We can catch the exception with...

catch (_com_error& err)

But even then we don't get much info other than something like unknown error.

As far as QLM is concerned I bet this is not a bug, but a feature.

I have an idea on how to handle this situation a bit better so we don't have to buy all of our devs a qlm license, I will try it out and report back.

K7ZCZ

2019-08-17 21:46

administrator   ~0008417

For sure, the call returning an error is a feature -- they want to defend the software to not be run under a debugger.

But that feature has a bug because the NULL return value can't be represented in _bstr_t, as coded in their TLB. If they want to be able to return NULL to indicate a problem, the TLB shouldn't be using _bstr_t -- at least, not in the way that it does. I think the com_error that you're finding is from the _bstr_t constructor against the NULL return value, not from an unsuccessful HRESULT or natrual exception from the API.

If we're completely convinced that unlicenced developent is the only way to get a NULL back here, then I don't think it's a particularly high-priority issue for us. But I have no reference which explains when or why the return value might be NULL, or how to get that value out of the TLB call-side wrapper without trauma. As such, I'm still worried that some user-capable configuration ends up causing a crash.

DOUG

2019-08-18 20:36

developer   ~0008422

So the isonline check was getting the valid QLM API error message on the non-licensed dev machines. So I made sure we didn't call the other web calls when isonline failed, which I should have been doing anyway.

Additionally,
IF we fail the isonline AND we are in DEBUG build AND we are in the debugger AND we are on a unlicensed dev machine, we will report the QLM error message one time.

Change set 5124

Note 1: If you do not have QLM licensed, you will have to license HRD outside the debugger one time. Subsequent runs will load fine.
Note 2: This should not be a risk to allow unlicensed HRD users to use the app by running in the debugger. The software will still require HRD to be licensed.

K7ZCZ

2019-08-20 12:28

administrator   ~0008424

Thanks! Your fix enables development on machines without a QLM development license.

I'm still a bit worried about this API returning NULL for other reasons, but I don't think that's an issue with much priority.

Issue History

Date Modified Username Field Change
2019-06-07 09:49 K7ZCZ New Issue
2019-06-07 09:49 K7ZCZ Status new => assigned
2019-06-07 09:49 K7ZCZ Assigned To => DOUG
2019-06-09 19:02 WA9PIE Module All => QLM
2019-06-09 19:04 WA9PIE Sub-Module (select) => Software License Key System
2019-06-10 23:33 DOUG Note Added: 0008036
2019-06-11 08:34 K7ZCZ Relationship added related to 0003345
2019-06-11 21:11 DOUG Note Added: 0008052
2019-06-11 21:11 DOUG Assigned To DOUG => K7ZCZ
2019-06-13 14:47 WA9PIE Module QLM => SW License Mgmt
2019-06-15 11:21 WA9PIE Description Updated View Revisions
2019-06-15 11:21 WA9PIE Sub-Module Software License Key System => SW License Client
2019-06-22 11:15 WA9PIE Note Added: 0008165
2019-07-21 06:38 K7ZCZ Relationship added related to 0003396
2019-07-21 06:46 K7ZCZ Note Added: 0008254
2019-07-21 06:46 K7ZCZ Status assigned => resolved
2019-07-21 06:46 K7ZCZ Resolution open => fixed
2019-08-15 22:42 K7ZCZ Note Added: 0008411
2019-08-15 22:48 K7ZCZ Assigned To K7ZCZ => DOUG
2019-08-16 12:15 K7ZCZ Note Added: 0008413
2019-08-16 21:38 DOUG Note Added: 0008414
2019-08-17 21:46 K7ZCZ Note Added: 0008417
2019-08-18 20:36 DOUG Note Added: 0008422
2019-08-20 12:28 K7ZCZ Note Added: 0008424
2019-09-14 02:19 WA9PIE Status resolved => closed
2019-09-14 02:19 WA9PIE Fixed in Version => 6.7.0.227
2019-09-14 02:19 WA9PIE Summary QLM license code incorrectly casts string, ignores error return from Socaro function => QLM license code incorrectly casts string, ignores error return from Soraco function
2019-09-14 02:19 WA9PIE Testing Not Started => Beta Successful
2019-11-08 02:13 WA9PIE Fixed in Version 6.7.0.227 => 6.7.0.244
2019-11-08 02:32 WA9PIE Project 3 - Current Dev List => Ham Radio Deluxe