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

Incomplete comparison with function strncmp #193

Closed
Daybreak2019 opened this issue May 27, 2021 · 4 comments
Closed

Incomplete comparison with function strncmp #193

Daybreak2019 opened this issue May 27, 2021 · 4 comments

Comments

@Daybreak2019
Copy link

Code snippet

static PyObject* solve(PyObject *self, PyObject *args, PyObject *kwrds)
{
      ............
      if (!PyArg_ParseTupleAndKeywords(args, kwrds, "OO|iiii", kwlist,
            &F, &B, &sys, &nrhs, &ldB, &oB)) return NULL;

      #if PY_MAJOR_VERSION >= 3
          if (!PyCapsule_CheckExact(F) || !(descr = PyCapsule_GetName(F)))
                 err_CO("F");
          if (strncmp(descr, "CHOLMOD FACTOR", 14))             -------> The string terminator is not considered.
                 PY_ERR_TYPE("F is not a CHOLMOD factor");
          cholmod_factor *L = (cholmod_factor *) PyCapsule_GetPointer(F, descr);
      #else
          if (!PyCObject_Check(F)) err_CO("F");
          descr = PyCObject_GetDesc(F);
          if (!descr || strncmp(descr, "CHOLMOD FACTOR", 14))    -------> The string terminator is not considered.
                 PY_ERR_TYPE("F is not a CHOLMOD factor");
          cholmod_factor *L = (cholmod_factor *) PyCObject_AsVoidPtr(F);
      #endif
      ............

Description

Function:
solve/spsolve/diag/getfactor
Call-path:
1. solve (Python) -> solve -> strncmp
2. spsolve (Python) -> spsolve -> strncmp
3. diag(Python) -> diag -> strncmp
4. getfactor(Python) -> getfactor-> strncmp
WarningType: Incomplete comparison.
Out analysis tool reported four warnings about the incomplete comparison of strings as shown above.
When the comparison length is 14, the terminator would be ignored. Hence even the strncmp returns 0, the reality may not match expectations specifically when variable descr depends on external inputs (Python).
For example, descr = "CHOLMOD FACTORMalicious", the comparison still return 0.
Also seen in solve, spsolve, diag and getfactor

@Daybreak2019
Copy link
Author

Anyone can help confirm this issue? thanks.

@kalvdans
Copy link

Yes, it looks like a bug to me too. We should indeed change to strcmp or include the trailing NUL byte in the comparison.

@sanurielf
Copy link

sanurielf commented Jul 30, 2021

I think best solution will be to implement strcmp(str1, str2) != 0 . I will do a PR on CVXOPT and also on my own fork (KVXOPT) with that change.

@sanurielf
Copy link

I just checked this part of the code could be updated to use TypeCheck_Capsule. Having that, no string comparison is needed. You can check that umfpack.c has this logic already

    if (!PyCapsule_CheckExact(F)) err_CO("F");
    if (SP_ID(A) == DOUBLE) {
        TypeCheck_Capsule(F, descrdFn, "F is not the UMFPACK numeric factor "
            "of a 'd' matrix");
    }
    else  {
        TypeCheck_Capsule(F, descrzFn, "F is not the UMFPACK numeric factor "
            "of a 'z' matrix");
    }

sanurielf added a commit to sanurielf/kvxopt that referenced this issue Aug 2, 2021
martinandersen added a commit that referenced this issue Sep 20, 2021
sanurielf added a commit to sanurielf/kvxopt that referenced this issue Sep 25, 2021
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