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

cfg: add library configuration for libselinux #6461

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented May 29, 2024

There are a couple false-positives and false-negatives:

  • no warning of ignoredReturnValue for get_default_type():

    <function name="get_default_type">
      <returnValue type="int"/>
      <noreturn>false</noreturn>
      <use-retval/>
      <leak-ignore/>
      <arg nr="1" direction="in">
        <not-null/>
        <not-uninit/>
        <strz/>
      </arg>
      <arg nr="2" direction="out">
        <not-null/>
      </arg>
    </function>
    <memory>
      <alloc init="true" arg="2">get_default_type</alloc>
      <dealloc>free</dealloc>
    </memory>
    get_default_type("object_r", type2);  // does not report ignoredReturnValue
  • wrong constVariablePointer report for selabel_open(), especially since the cleanup function selabel_close() does take a not pointer to non-const:

    <function name="selabel_open">
      <returnValue type="struct selabel_handle *"/>
      <noreturn>false</noreturn>
      <use-retval/>
      <leak-ignore/>
      <arg nr="1" direction="in">
        <not-uninit/>
        <not-bool/>
        <valid>0:5</valid>
      </arg>
      <arg nr="2" direction="in">
        <not-uninit/>
        <minsize type="argvalue" arg="3"/>
      </arg>
      <arg nr="3" direction="in">
        <not-uninit/>
        <not-bool/>
      </arg>
    </function>
    struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);  // reports constVariablePointer
  • missing memleak for getseuserbyname():

    <function name="getseuserbyname">
      <returnValue type="int"/>
      <noreturn>false</noreturn>
      <use-retval/>
      <leak-ignore/>
      <arg nr="1" direction="in">
        <not-null/>
        <not-uninit/>
        <strz/>
      </arg>
      <arg nr="2" direction="out">
        <not-null/>
      </arg>
      <arg nr="3" direction="out">
        <not-null/>
      </arg>
    </function>
    <memory>
      <alloc init="true" arg="2">getseuserbyname</alloc>
      <dealloc>free</dealloc>
    </memory>
    <memory>
      <alloc init="true" arg="3">getseuserbyname</alloc>
      <dealloc>free</dealloc>
    </memory>
    void getseuserbyname_fail2(void)
    {
      char *seuser, *level;
      getseuserbyname("root", &seuser, &level);
      free(level);
    
      // seuser is leaked; no memleak report
    }

@firewave
Copy link
Collaborator

Thanks for your contribution.

You also need to install the library in the CI job in CI-unixish.yml which runs the test in strict mode.

Also the configuration seems to have an unclosed comment.

@cgzones
Copy link
Contributor Author

cgzones commented May 29, 2024

You also need to install the library in the CI job in CI-unixish.yml which runs the test in strict mode.

Done.

Also the configuration seems to have an unclosed comment.

True, fixed.

Any ideas for the above issues?

@chrchr-github
Copy link
Collaborator

chrchr-github commented May 29, 2024

struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);  // reports constVariablePointer

This should depend on how hnd is used later on. It's not clear if this is a false positive yet.
It looks like a true positive in void selabel_fail1(void),

@chrchr-github
Copy link
Collaborator

chrchr-github commented May 29, 2024

get_default_type("object_r", type2);  // does not report ignoredReturnValue

It is reported if the <memory> configuration is removed. So I suppose we might suppress the ignoredReturnValue in favor of memleak (which should only happen for functions which return the allocated pointer).
Could be fixed by adding

const bool warn = [&]() {
    if (tok->function() && (tok->function()->isAttributeNodiscard() || tok->function()->isAttributePure() || tok->function()->isAttributeConst()))
        return true;
    // avoid duplicate warnings for resource-allocating functions
    if (retvalTy == Library::UseRetValType::DEFAULT) {
        if (const Library::AllocFunc* info = mSettings->library.getAllocFuncInfo(tok))
            return info->arg > 0;
    }
    return false;
    }();

in checkIgnoredReturnValue().
Edit: memleak works as expected.
Outparam allocations are supported, but maybe not always handled correctly. See e.g. 63a5a71

@chrchr-github
Copy link
Collaborator

void getseuserbyname_fail2(void)
{
  char *seuser, *level;
  getseuserbyname("root", &seuser, &level);
  free(level);

  // seuser is leaked; no memleak report
}

Looks like Library::AllocFunc supports only one arg.

@@ -449,26 +449,7 @@ function cppunit_fn {

# selinux.c
function selinux_fn {
if [ $HAS_PKG_CONFIG -eq 1 ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because libselinux does not make use of pkg-config

@chrchr-github
Copy link
Collaborator

struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);  // reports constVariablePointer

This should depend on how hnd is used later on. It's not clear if this is a false positive yet. It looks like a true positive in void selabel_fail1(void),

The FP involving selabel_lookup() can be fixed by removing the argument direction for hnd. #6451 might improve that situation.

@cgzones
Copy link
Contributor Author

cgzones commented Jun 7, 2024

struct selabel_handle *hnd = selabel_open(SELABEL_CTX_FILE, NULL, 0);  // reports constVariablePointer

This should depend on how hnd is used later on. It's not clear if this is a false positive yet. It looks like a true positive in void selabel_fail1(void),

The FP involving selabel_lookup() can be fixed by removing the argument direction for hnd. #6451 might improve that situation.

Thanks; changed the argument direction in selabel_lookup() to inout

@chrchr-github
Copy link
Collaborator

Maybe we should add include detection here:

include_mappings = {'boost': ['<boost/'],

@danmar
Copy link
Owner

danmar commented Jun 11, 2024

Maybe we should add include detection here:

include_mappings = {'boost': ['<boost/'],

sorry is this related to this PR?

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@chrchr-github
Copy link
Collaborator

Maybe we should add include detection here:

include_mappings = {'boost': ['<boost/'],

sorry is this related to this PR?

If we could detect the corresponding headers, we would automatically load the new library in daca (as far as I understand it). Same with the emscripten PR #6503

@firewave
Copy link
Collaborator

If we could detect the corresponding headers, we would automatically load the new library in daca (as far as I understand it). Same with the emscripten PR #6503

Those need to be disabled though if this library did not existing in the previous stable version or we will get a failure.

Also please remember to bump the version of the script when changing it.

@chrchr-github
Copy link
Collaborator

Those need to be disabled though if this library did not existing in the previous stable version or we will get a failure.

So we should add it, but leave it commented out until 2.15 is released?

@firewave
Copy link
Collaborator

So we should add it, but leave it commented out until 2.15 is released?

Yes.

@chrchr-github
Copy link
Collaborator

There is a branch conflict now.

@firewave
Copy link
Collaborator

This is also still missing the library detection in daca.

@firewave
Copy link
Collaborator

firewave commented Jul 8, 2024

Thanks. Please update CLIENT_VERSION in tools/donate_cpu_lib.py as well.

@chrchr-github
Copy link
Collaborator

chrchr-github commented Jul 13, 2024

Thanks. Please update CLIENT_VERSION in tools/donate_cpu_lib.py as well.

AFAIK we intended to leave the detection commented out until 2.15 is released. Not sure if the version number needs to be bumped if there is no functional change.

tools/donate_cpu_lib.py Outdated Show resolved Hide resolved
@firewave
Copy link
Collaborator

AFAIK we intended to leave the detection commented out until 2.15 is released.

Good catch,

Not sure if the version number needs to be bumped if there is no functional change

For this file I would prefer it to get an indication on how old the client actually is.

@chrchr-github chrchr-github merged commit 81e7614 into danmar:main Jul 22, 2024
9 checks passed
@chrchr-github
Copy link
Collaborator

@cgzones How do you wish to be credited in the Authors file?

@cgzones
Copy link
Contributor Author

cgzones commented Jul 24, 2024

How do you wish to be credited in the Authors file?

As 'Christian Göttsche'.

Thanks for your support and this amazing tool in general.

@cgzones cgzones deleted the selinux branch July 24, 2024 15:35
@chrchr-github
Copy link
Collaborator

How do you wish to be credited in the Authors file?

As 'Christian Göttsche'.

Done: 15bdd97

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

Successfully merging this pull request may close these issues.

4 participants