SUTypedValueGetArrayItems() has unexpected behaviour

[TommyKaneko]

Bug on SU 2018 API, Mac and Windows

The 'array' of SUTypedValueRefs returned by SUTypedValueGetArrayItems() has unexpected behaviour. I have noticed that releasing the first SUTypedValueRef item in the array seems to release all subsequent SUTypedValueRef objects in the array.

The issue was picked up through the SU C++ Wrapper. This bug is especially problematic, as it prevents SUTypedValueRef objects within an array to be treated as distinct objects as one would want with C++.

More details here:
TommyKaneko/Sketchup-API-C-Wrapper#19 (comment)

I hope I am correct in my analysis - it took a while to figure this out.

[jimfoltz]

This may or may not be related - calling SUTypedValueGetArrayItems() doesn't set the 4th parameter which is meant to be the number of elements returned. The output array is populated, but the count is always 0 for me.

Source example

[thomthom]

This may or may not be related - calling SUTypedValueGetArrayItems() doesn't set the 4th parameter which is meant to be the number of elements returned. The output array is populated, but the count is always 0 for me.

I can confirm that the source code isn't setting that count even though it populate the array. That's a bug for sure. Though that is separate from Tommy's reported issue.

(Btw, when creating a vector to be filled by the C API it's generally best to explicitly set all the content to SU_INVALID; std::vector<SUTypedValueRef> refs(len, SU_INVALID);)

Jim, another side comment on your example; https://github.com/jimfoltz/jf-sketchup-c-api-sandbox/blob/exp/typed-value-array-example/src/main.cpp#L40

    SU_CALL(SUTypedValueGetString(tv, &s));
    size_t s_len = 0;
    SU_CALL(SUStringGetUTF8Length(s, &s_len));
    size_t out = 0;
    std::string str{};
    SUStringGetUTF8(s, s_len + 1, &str[0], &out);

You are creating an empty std::string with std::string str{} - then feed that to SUStringGetUTF8. SUStringGetUTF8 expects a pre-allocated buffer to fill the string. In this case it's not. You might be getting away with really short strings as std::string due to small string optimizations.

But doing it this way makes it write outside the bounds of memory you allocated.

Here's a helper function I use:

std::string GetString(const SUStringRef& string) {
  size_t length = 0;
  SU(SUStringGetUTF8Length(string, &length));
  std::vector<char> buffer(length + 1);
  size_t out_length = 0;
  SU(SUStringGetUTF8(string, length + 1, buffer.data(), &out_length));
  assert(out_length == length);
  return std::string(begin(buffer), end(buffer));
}

[thomthom]

@TommyKaneko - can you provide a minimal code example and update your original post please. It's unclear to me exactly what is being observed and done here.

[TommyKaneko]

@thomthom Code example is here:
https://github.com/TommyKaneko/SU-Sandbox/blob/master/src/SUTypedValueGetArrayItems_test/main.cpp

The problem is on line 37. The problem being that each TypedValueRef returned cannot be released independently.

[thomthom]

I can reproduce - not sure what is causing it.

However, here is a version that doesn't have memory leaks:

int _tmain(int argc, _TCHAR* argv[])
{
  std::string project_path = "C:\\Users\\tthomas2\\SourceTree\\SLAPI-Test";
  std::string model_path = project_path + "\\models\\issue-19.skp";
  std::string temp_path = project_path + "\\temp";

  SUInitialize();

  SUModelRef model = SU_INVALID;
  SU(SUModelCreateFromFile(&model, model_path.c_str()));

  SUAttributeDictionaryRef dict_ref =  SU_INVALID ;
  SUResult res = SUModelGetAttributeDictionary(model, "TypedValues", &dict_ref);
  
  SUTypedValueRef val = SU_INVALID;
  SUTypedValueCreate(&val);
  const char* key_char = "Array";
  res = SUAttributeDictionaryGetValue(dict_ref, &key_char[0], &val);
  assert(res == SU_ERROR_NONE);

  size_t count = 0;
  res = SUTypedValueGetNumArrayItems(val, &count);
  assert(res == SU_ERROR_NONE);
  std::vector<SUTypedValueRef> values(count, SU_INVALID);
  res = SUTypedValueGetArrayItems(val, values.size(), values.data(), &count);
  assert(res == SU_ERROR_NONE);

  for (auto& value : values) {
    // Do stuff with the typed Values
    SUTypedValueType type;
    res = SUTypedValueGetType(value, &type);
    std::cout << "TypedValue type is No: " << type << "\n";
    // Then release each one
    SUTypedValueRelease(&value); // CRASH on second pass - indicates TypedValue has already been released.
  }

  SU(SUModelRelease(&model));

  SUTerminate();
  return 0;
}

[thomthom]

Looking close at this, the first SUTypedValueRelease doesn't have any relevance.
If you directly try to release the second item; SUTypedValueRelease(&values[1]); it still crashes.

Seems like only the first item from SUTypedValueGetArrayItems is valid.

[thomthom]

@BugraBarin - is SUTypedValue objected returned from SUTypedValueGetArrayItems supposed to be released? The documentation only mentions to use SUTypedValueRelease after using SUTypedValueCreate.

Is the elements returned from SUTypedValueGetArrayItems owned by the containing SUTypeRef?

I only saw a couple of usages of SUTypedValueGetArrayItems and none of them released the typed values the array.

[TommyKaneko]

I only saw a couple of usages of SUTypedValueGetArrayItems and none of them released the typed values the array.

Is this acceptable behaviour in your view? From the C++ Wrapper perspective, this becomes tricky, as some TypedValue objects need to be released on object destruction, and others in this case do not. Would prefer consistent behaviour.

Or perhaps we should think of an Array of TypedValue objects as being "attached" to the original TypedValue that it was extracted from?

[thomthom]

I've not used SUTypedValue myself - first time I looked at the code.

Or perhaps we should think of an Array of TypedValue objects as being "attached" to the original TypedValue that it was extracted from?

I'm starting to wonder if this is the case - like getting a face from the model. But need to dig deeper. It isn't completely obvious to me either. This is either a bug or a documentation issue.

[thomthom]

This may or may not be related - calling SUTypedValueGetArrayItems() doesn't set the 4th parameter which is meant to be the number of elements returned. The output array is populated, but the count is always 0 for me

Logged as SU-38670

[BugraBarin]

I wasn't very familiar with this function either but it looks like the typed values contained within an "array type" typed value is owned by that typed value. So the contained typed values must not be released individually. Only the container typed value need to be released. We certainly need to make this clear in the docs.

But, you could look at it this way: Since you didn't actually allocate the typed values yourself (via SUTypedValueCreate), you shouldn't release them either. You are only allocating the references there, the actual data is populated by (and owned by) the parent SUTypedValueRef (which is an "array" type).

I know this is a bit of a mind bender. Let me know if anything is unclear.

[TommyKaneko]

I can understand that reasoning. I think this should be made clear in the docs in that case.

[jimfoltz]

Thanks for the code & comments, @thomthom . Care to share your SU macro?

[thomthom]

@jimfoltz Sure thing. It's just a different take on handling errors. I'm not so keen on automatically throwing so I assert instead:

#include <assert.h>

#define SU(api_function_call) {\
  SUResult su_api_result = api_function_call;\
  assert(SU_ERROR_NONE == su_api_result);\
}\

The throwing is fine is you expect you app to crash upon failure. But if you start catching the errors to recover you'll probably leak memory at some point. (Unless you use Tommy's C++ wrapper which should ensure memory is managed - thus cleaned up in destructors .)

Additionally - I use this in combination to test the C API - in which case I some times want to continue executing the code. but have the debugger stop on the asserts.

[thomthom]

Bugra have already submitted a documentation improvement to these functions. Should be available on the next SDK drop.