0

The following code worked just fine thank you with one COM client, but with a new client (the updated version of the same software) string_array_to_bstr_safearray_variant throws an access violation and everything dies.

Can anyone tell me if I'm doing something wrong that I was getting away with before..? Am I failing to allocate memory properly?

#include "comutil.h"

void string_array_to_bstr_safearray_variant(long arraylength,char ** in_array,VARIANT *out_variant)
{
    CComSafeArray<BSTR> out_array;
    ATLENSURE_SUCCEEDED(out_array.Create(arraylength));
    for (int i=0;i<arraylength;i++)
        ATLENSURE_SUCCEEDED(out_array.SetAt(i,_com_util::ConvertStringToBSTR(in_array[i])));
    CComVariant ccv(out_array);
    HRESULT hr = ccv.Detach(out_variant);
    ATLENSURE_SUCCEEDED(hr);
}

//names: output parameter to contain variant holding safearray of bstrs
STDMETHODIMP CCalculation::get_output_shortnames(VARIANT* names)
{
    char** names_array = calc_get_short_output_names(calc); //this works fine
    string_array_to_bstr_safearray_variant(output_length,names_array,names); //this fails before returning
    return S_OK;
}

Edit: Debugger info

Without a debugger, I get an access violation.

Stepping through this code with the debugger it appears to work. output_length is set correctly; out_array is created and filled correctly and so is out_variant as far as I can tell through variable watching. However, the COM client still fails, saying "lisp value has no coercion to VARIANT with this type: #<safearray...>" (which is odd because a previous version of the client interprets the return value just fine). It then crashes complaining that it's out of memory.

Running the code inside the debugger, but letting it run rather than stepping, it failed inside the constructor of CComVariant, complaining of an invalid argument thrown because the inner call to SafeArrayCopy failed.

Edit: on another recent step-through it failed in the loop, so maybe the problem is with CComSafeArray as @terriblememory suggests?

Sideshow Bob
  • 4,566
  • 5
  • 42
  • 79
  • COM, SafeArrays & BSTRs... Oh my! – John Dibling May 04 '12 at 13:14
  • The dark clouds parted and a thunderbolt of lightning struck, giving the *output_length* variable a value. It's bound to be a bit crisp at the edges. Don't write functions that return arrays that don't also return the *array length*. They are fundamentally unsafe. A vector<> is an obvious choice. – Hans Passant May 04 '12 at 13:31
  • Lol :) Yes, `output_length` is a class member and is valid by the time `get_output_shortnames()` gets called. `calc_get_short_output_names` is a C interface. That isn't the bug. – Sideshow Bob May 04 '12 at 14:42
  • Then you are just going to have to use a debugger to see what the code is actually doing at runtime at each step. It would also help you determine which line of code is throwing the AV in the first place. – Remy Lebeau May 04 '12 at 17:01
  • `calc_get_short_output_names is a C interface`. How does C code set a C++ class member? I'm just not buying this. The 95% diagnostic for looking at code and thinking "that can't ever work" is that it doesn't actually work because it can't work. The other 5% is code that should never have been written. – Hans Passant May 06 '12 at 01:16
  • `output_length` is set in the `init()` method of `CCalculation`. It applies to several arrays besides that returned by `calc_get_short_output_names` and will be valid so long as `init()` was called, which it was. @HansPassant if this code falls within your "5% that should never have been written", fine. I wonder if you have any ideas on why the debugger says what it does though? – Sideshow Bob May 08 '12 at 10:51
  • (See updated debug info above - thanks @RemyLebeau) – Sideshow Bob May 08 '12 at 10:52
  • Is there any reason to ignore `HRESULT`s? It just like flying into fog. You should be using `ATLVERIFY(SUCCEEDED(` or `ATLENSURE_SUCCEEDED(` to unconditionally exclude trivial problems. – Roman R. May 08 '12 at 11:17
  • BTW if you hit out of memory issue, then you could have checked if you indeed run out of memory. Stack memory, virtual address space etc. You might be having a million strings there, or one of the strings is actually a trash pointer causing allocation of a very long string. – Roman R. May 08 '12 at 11:23
  • What is the value of arraylength? Something sane, like 100 or more like 1,000,000,000? – Ben May 08 '12 at 11:33
  • Sane - around 60 (and the strings are never more than 20 chars long either) – Sideshow Bob May 08 '12 at 15:49
  • If `SafeArrayCopy()` is failing, then chances are the BSTR safearray is not being constructed correctly in the first place. – Remy Lebeau May 08 '12 at 18:10
  • Ok. I now check HRESULTS (see code update). They are fine, but the constructor of CComSafeArray fails. I checked actual memory usage, no issues there. – Sideshow Bob May 09 '12 at 17:42

2 Answers2

1

The documentation for CComSafeArray doesn't actually say it supports BSTRs. Is the feature flag for BSTRs being set in the underlying SAFEARRAY? (This isn't really an answer, but I don't have the karma to just comment, sorry!)

terriblememory
  • 1,712
  • 3
  • 18
  • 25
  • Good question. Well, the feature flags that are set are `FADF_BSTR` (all well and good) and `FADF_HAVEVARTYPE`. I'm not sure what the second one is, but msdn says *"If the fFeatures field contains FADF_HAVEVARTYPE, the cLocks field MUST contain a VARIANT type constant in its high word that specifies the type of the elements in the array. Otherwise, the high word of the cLocks field MUST be set to 0."* `cLocks` is set to `1` so its high word must be zero which as a variant type is `VT_EMPTY` http://msdn.microsoft.com/en-us/library/cc237865%28v=prot.13%29.aspx could that be wrong? – Sideshow Bob May 09 '12 at 18:07
  • Interesting. I suspect that documentation is wrong - it looks like a copy & paste of the FADF_HAVEIID entry. I would guess it should really say that there's a VARIANTTYPE at offset -4. At any rate it does seem reasonable that it's set. – terriblememory May 09 '12 at 19:21
  • Hang on, don't you need to detach the out_array into the out_variant? i.e. CComVariant ccv(out_array.Detach()); – terriblememory May 09 '12 at 19:42
  • Looking at the docs you'd think so (though the ATLSafeArray sample http://msdn.microsoft.com/en-us/library/akdhw818%28v=vs.80%29.aspx doesn't do it). Tried it though, and alas, still broken. Could it be a bug in the COM client perhaps? – Sideshow Bob May 10 '12 at 10:52
1

Well finally I found the answer to this! The code posted in the question is correct as it is. There was earlier code causing undefined behaviour:

STDMETHODIMP CCalculation::configure(VARIANT radii) // radii contains a safearray of doubles
{
CComSafeArray<double> radii_sa;
radii_sa.Attach(radii.parray);
ULONG num_radii = radii_sa.GetCount();

//unpack radii array into c-style array
double *radii_array = new double[num_radii];
for (long i=0;i<num_radii;i++)
radii_array[i] = radii_sa.GetAt(i);

//...do something with radii_array...

delete[] radii_array;

return S_OK;
}

Did you spot the deliberate mistake? COM rules say that radii is owned by the client, not my dll. By attaching to it then letting the wrapper go out of scope, I was deallocating the safearray. Fixed by adding this before the return statement:

radii_sa.Detach();
Sideshow Bob
  • 4,566
  • 5
  • 42
  • 79