python/pysss_nss_idmap: check return from functions

[ikerexxe]

Coverity warns that PyModule_AddIntConstant() returns operation success
or failure but this value is never checked.

Error: CHECKED_RETURN (CWE-252):
sssd-2.3.0/src/python/pysss_nss_idmap.c:587: check_return: Calling
"PyModule_AddIntConstant" without checking return value (as is done
elsewhere 4 out of 5 times).
sssd-2.3.0/src/python/pyhbac.c:1956: example_assign: Example 1:
Assigning: "ret" = return value from "PyModule_AddIntConstant(m,
"HBAC_CATEGORY_ALL", 1L)".
sssd-2.3.0/src/python/pyhbac.c:1957: example_checked: Example 1 (cont.):
"ret" has its value checked in "ret == -1".
sssd-2.3.0/src/python/pyhbac.c:1960: example_assign: Example 2:
Assigning: "ret" = return value from "PyModule_AddIntConstant(m,
"HBAC_RULE_ELEMENT_USERS", 1L)".
sssd-2.3.0/src/python/pyhbac.c:1961: example_checked: Example 2 (cont.):
"ret" has its value checked in "ret == -1".
sssd-2.3.0/src/python/pyhbac.c:1972: example_assign: Example 3:
Assigning: "ret" = return value from "PyModule_AddIntConstant(m,
"HBAC_EVAL_DENY", HBAC_EVAL_DENY)".
sssd-2.3.0/src/python/pyhbac.c:1973: example_checked: Example 3 (cont.):
"ret" has its value checked in "ret == -1".
sssd-2.3.0/src/python/pyhbac.c:1982: example_assign: Example 4:
Assigning: "ret" = return value from "PyModule_AddIntConstant(m,
"HBAC_ERROR_NOT_IMPLEMENTED", HBAC_ERROR_NOT_IMPLEMENTED)".
sssd-2.3.0/src/python/pyhbac.c:1983: example_checked: Example 4 (cont.):
"ret" has its value checked in "ret == -1".
 #  585|       PyModule_AddIntConstant(module, "ID_NOT_SPECIFIED",
 #  586|                               SSS_ID_TYPE_NOT_SPECIFIED);
 #  587|->     PyModule_AddIntConstant(module, "ID_USER", SSS_ID_TYPE_UID);
 #  588|       PyModule_AddIntConstant(module, "ID_GROUP", SSS_ID_TYPE_GID);
 #  589|       PyModule_AddIntConstant(module, "ID_BOTH", SSS_ID_TYPE_BOTH);

Moreover, even though coverity doesn't indicate it the same happens with
PyModule_AddStringConstant().

[alexey-tikhonov]

Thanks for the update. Please see comment inline.

Btw, wouldn't it make sense to include Py_DECREF(module); (actually better Py_XDECREF) into MODINITERROR?

[ikerexxe]

Thanks for the update. Please see comment inline.

Yes, it makes sense. I'll change it.

Btw, wouldn't it make sense to include Py_DECREF(module); (actually better Py_XDECREF) into MODINITERROR?

It makes sense to include Py_XDECREF(module) in another macro called FREEMOD_AND_RETURN(?) and call it from the changes I've made. I wouldn't like messing with MODINITERROR as it's already called from other places where it doesn't make to decrease module. An example:

sssd/src/util/sss_python.h

Line 39 in fbc7082

MODINITERROR; \

What do you think?

[alexey-tikhonov]

I think you just spotted yet another one place that needs fixing: TYPE_READY calls PyModule_AddObject() and doesn't check a result.

Moreover, taking into account where this macro (TYPE_READY) is used, Py_XDECREF should be called there as well, IMO.

[ikerexxe]

I think you just spotted yet another one place that needs fixing: TYPE_READY calls PyModule_AddObject() and doesn't check a result.

Moreover, taking into account where this macro (TYPE_READY) is used, Py_XDECREF should be called there as well, IMO.

Agreed. Take a look to the latest update, which contains all the aforementioned errors fixed.

[alexey-tikhonov]

Why do we need both MODINITERROR and FREEMOD_AND_RETURN?

I still think we can have single macro (calling Py_XDECREF on NULL should be safe) and use it everywhere.

[alexey-tikhonov]

Thank you for the updated version. Please see a couple of comments inline.

[alexey-tikhonov]

Thank you. ACK.

[pbrezina]

Pushed PR: #5226

• master
  • 03b00f7 - python/pysss: if PyModule* fails decrement references
  • 8b1a8cf - python/pyhbac: if PyModule* fails decrement references
  • c008d89 - python/pysss_nss_idmap: check return from functions
  • 838baa8 - util/sss_python: change MODINITERROR to dereference module