Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using SoapySDRArgInfoList_clear() on an ArgInfoList results in invalid free() #361

Closed
staticfloat opened this issue Jun 19, 2022 · 7 comments

Comments

@staticfloat
Copy link
Contributor

We built a SoapySDR loopback module for internal testing, based off of the rtlsdr codebase. During implementation, we borrowed the structure of functions such as getSettingInfo, however we've noticed that, after obtaining the argument info lists via SoapySDRDevice_getSettingInfo(), if we then try to free the list via SoapySDRArgInfoList_clear(), there is an invalid free() triggered upon the freeing of the string pointers. We believe this is due to the usage of string literals being assigned to the char * elements of the ArgInfoList structure, followed by these free() calls.

I see two possible ways forward:

  • We could pass all string literals through strdup() such that those free() calls are operating on actually allocated memory.
  • We could eliminate the free() calls somehow; either by never freeing such strings, or by embedding a bitmap that enables/disables freeing of certain pieces of memory.

In practice, I doubt many users are calling this function in a loop, so perhaps the memory accounting here is not so necessary for usual use. I just hate to see a memory leak anywhere.

@zuckschwerdt
Copy link
Member

IIRC initializing a std::string using a string literal will always involve a copy. The C++ side of things should be good?

On the C side we have toArgInfoList(), using toArgInfo(), using toCString(), and there is an alloc there, right?

So SoapySDRArgInfo_clear() and SoapySDRArgInfoList_clear() shouldn't be any trouble?

@zuckschwerdt
Copy link
Member

Oh, I see you say "borrowed". Note that fooArg.key = "foo_key"; in C++ translates to fooArg->key = strdup("foo_key"); in C.

@staticfloat
Copy link
Contributor Author

Ah, that might be part of the problem; we're using the C bindings (since we are coming from a higher-level language) so we don't actually call the C++ functions, we call the C functions directly. I assumed that this was the preferred method of interacting with libSoapySDR if coming from a non-C++-native language?

@guruofquality
Copy link
Contributor

So in C++, SoapySDR::ArgInfoList SoapyRTLSDR::getSettingInfo(void) const the result is held in a structure full of std::vector and std::string types that are freed on scope exit

Now for the C bindings, SoapySDRArgInfo *SoapySDRDevice_getSettingInfo(const SoapySDRDevice *device, size_t *length) the result is held in a pointer of structures that must be freed by SoapySDRArgInfoList_clear

I assumed that this was the preferred method of interacting with libSoapySDR if coming from a non-C++-native language?

In either language, the caller owns the result. Either is fine, I dont think there is a preferred option.

We believe this is due to the usage of string literals being assigned to the char * elements of the ArgInfoList structure, followed by these free() calls.

It should not matter however the getSettingInfo was implemented, const strings or not. The result is actually copied into a std::string held by the structure in C++. And then for the C bindings, that std::string is copied again into the C structure/arrays with strdup()

if we then try to free the list via SoapySDRArgInfoList_clear(), there is an invalid free() triggered upon the freeing of the string pointers.

I dont think the cause is const strings in the driver, but there could be a bug. Do you have a backtrace of the failed call to 'free()'? Is there any chance some prior code called free() on the pointer returned by SoapySDRDevice_getSettingInfo?

@zuckschwerdt
Copy link
Member

Note the "borrowed", I assume that SoapySDRArgInfo struct was filled by custom code and of course SoapySDRArgInfoList_clear() croaks on invalid pointers then. It's not a bug in Soapy but a misunderstanding on implementing the API. Thus my hint:

... translates to fooArg->key = strdup("foo_key"); in C.

@staticfloat
Copy link
Contributor Author

Sorry, I dug into this further thanks to your pointers, and I noted that we ourselves were accidentally freeing one of the strings in the object within one of our debugging routines. Terribly sorry for the noise!

@staticfloat
Copy link
Contributor Author

X-ref: JuliaTelecom/SoapySDR.jl#58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants