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

Access Violation in SoapySDRArgInfo #447

Open
VE3NEA opened this issue Feb 13, 2025 · 4 comments
Open

Access Violation in SoapySDRArgInfo #447

VE3NEA opened this issue Feb 13, 2025 · 4 comments

Comments

@VE3NEA
Copy link

VE3NEA commented Feb 13, 2025

Problem Description

The C wrapper around SoapySDR::ArgInfo looks like this:

static inline SoapySDRArgInfo toArgInfo(const SoapySDR::ArgInfo &info)
{
    SoapySDRArgInfo out;
    std::memset(&out, 0, sizeof(out));
    try
    {
      ...
        out.optionNames = toStrArray(info.optionNames, &out.numOptions);
        // do options after optionNames so correct numOptions is reported if no optionNames
        out.options = toStrArray(info.options, &out.numOptions);

When info.options has elements but info.optionNames doesn't, out.optionNames is assigned a pointer to a zero-length memory. The calling code has no way to know that, when it tries to read numOptions entries from that memory, an AV exception is fired.

Proposed Fix

Change toStrArray to return NULL when strs has no entries

static inline char **toStrArray(const std::vector<std::string> &strs, size_t *length)
{
  if (strs.size() == 0) return NULL;
  ...

and make a corresponding change inSoapySDRStrings_clear:

void SoapySDRStrings_clear(char ***elems, const size_t length)
{
  if (*elems == NULL) return;
  ...
@guruofquality
Copy link
Contributor

Looks related to https://github.com/pothosware/SoapySDR/pull/444/files

Thanks for pointing this out. My thoughts about this

  1. Maybe optionNames should explicitly be set to nullptr when info.optionNames is empty, no change to the strings conversion

  2. when info.optionNames is empty, optionNames becomes a clone of options instead. The point was to just for optionNames to be a displayable name for options, so this fits the purpose.

@VE3NEA
Copy link
Author

VE3NEA commented Feb 13, 2025

I like the second option more as it does not require changes in the calling code to handle nulls. The previous fix returns an array of nulls if option names are not present, which also requires special handling in the calling code. So the fix will be

out.optionNames = toStrArray(info.optionNames.size() > 0 ? info.optionNames : info.options, &out.numOptions);

@zuckschwerdt
Copy link
Member

Will info.optionNames.size() > 0 really work? What if there are N options but 0 < M < N names?

@VE3NEA
Copy link
Author

VE3NEA commented Feb 13, 2025

The sdr driver is supposed to pass either two vectors of the same length, or an empty and non-empty vectors. If it returns 0 < M < N, it is a bug that needs to be fixed in the driver as in this case there is no reliable way to match the options and their names. We could change the condition to info.optionNames.size() == info.optios.size(), but this would just hide the error.

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