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.

Sign up for GitHub

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

Jump to bottom

Sanitizer test #321

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Sanitizer test #321

wants to merge 8 commits into from

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Oct 12, 2021

No description provided.

@cgzones
Copy link
Contributor Author

cgzones commented Oct 12, 2021

/cc @evverx

@evverx
Copy link
Contributor

evverx commented Oct 12, 2021

@cgzones Looks like UBSan no longer complains about those null pointers. Thanks!

Regarding memory leaks, unfortunately there're a lot of them in the tests themselves so I think until they're fixed they probably shouldn't be reported so as not to clutter the output of the tests.

@evverx
Copy link
Contributor

evverx commented Oct 12, 2021

I think until they're fixed they probably shouldn't be reported so as not to clutter the output of the tests.

Having said that, looking at the "raw" logs of the action I think it's more or less manageable so I'm going to have to take that back.

@evverx evverx mentioned this pull request Oct 12, 2021
Closed
@cgzones cgzones force-pushed the sanitizer_test branch 4 times, most recently from 844fb24 to 829f618 Compare October 13, 2021 12:45
@evverx
Copy link
Contributor

evverx commented Oct 14, 2021

@cgzones I noticed that you sent the patches where all the issues found by ASan/UBsan were fixed to the mailing list. Are you planning to send the other commits later or should I just cherry-pick the two commits you added related to GHActions on top of my PR and send it myself?

@cgzones
Copy link
Contributor Author

cgzones commented Oct 15, 2021

Are you planning to send the other commits later or should I just cherry-pick the two commits you added related to GHActions on top of my PR and send it myself?

I'd appreciate if you could send them yourself.
(Probably there is no need to cherry-pick my patches: ci: enable leak sanitizer is just a partial revert of ci: ignore memory leaks for now and ci: build all subdirectories in sanitizer job except python and sandbox is void, since all of those sub directories have no tests.)

@evverx
Copy link
Contributor

evverx commented Oct 15, 2021

I'll squash the commits into one and send it to the mailing list once https://lore.kernel.org/selinux/[email protected]/T/#t is merged to keep the GHActions workflow green. Thanks!

fishilico
fishilico reviewed Oct 16, 2021
View reviewed changes
libsemanage/tests/test_ibendport.c Outdated
@@ -362,6 +373,8 @@ void test_ibendport_modify_del_query_local(void)
&ibendport_local) < 0);

/* cleanup */
semanage_ibendport_key_free(key);
semanage_ibendport_free(ibendport_local);
Copy link
Member

@fishilico fishilico Oct 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@cgzones This semanage_ibendport_free(ibendport_local); should probably be moved after CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local); (a few lines up), as the variable is then used again in the test which checks that semanage_ibendport_query_local fails,

	CU_ASSERT(semanage_ibendport_query_local(sh, key,
						 &ibendport_local) < 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, I'll reorder this and the other affected free calls

fishilico
fishilico reviewed Oct 16, 2021
View reviewed changes
libsemanage/tests/test_user.c Outdated
@@ -577,6 +590,8 @@ void test_user_modify_del_query_local(void)
CU_ASSERT(semanage_user_query_local(sh, key, &user_local) < 0);

/* cleanup */
semanage_user_key_free(key);
semanage_user_free(user_local);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as 322243e#r730306247 here: semanage_user_free(user_local); should be moved up a few lines, before CU_ASSERT(semanage_user_query_local(sh, key, &user_local) < 0);

fishilico
fishilico reviewed Oct 16, 2021
View reviewed changes
libsemanage/tests/test_iface.c Outdated
@@ -546,6 +567,8 @@ void test_iface_modify_del_query_local(void)
CU_ASSERT(semanage_iface_query_local(sh, key, &iface_local) < 0);

/* cleanup */
semanage_iface_key_free(key);
semanage_iface_free(iface_local);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as 322243e#r730306247 here: semanage_iface_free(iface_local); should be moved up a few lines, before CU_ASSERT(semanage_iface_query_local(sh, key, &iface_local) < 0);

@cgzones
Copy link
Contributor Author

cgzones commented Nov 16, 2021

@evverx fyi, the sanitizer patches have been applied via b98d3c4, ea53901, fe01a91 and f95dbf2

@evverx
Copy link
Contributor

evverx commented Nov 16, 2021 •
edited
Loading

@cgzones thanks! I'll try to rebase #320 on top of the master branch to make sure it works, squash the commits into one and send it to the mailing list.

evverx added a commit to evverx/selinux that referenced this pull request Nov 16, 2021
@evverx
ci: run the tests under ASan/UBsan on GHActions
5dbde4b 
It was tested in SELinuxProject#321 and
SELinuxProject#320. In the process
it discovered a few issues all of which were fixed in
SELinuxProject@b98d3c4
SELinuxProject@ea53901
SELinuxProject@fe01a91
SELinuxProject@f95dbf2

Now that all the issues are gone it should be safe to turn it on
to make it easier to automatically catch bugs like that almost as soon as
they end up in the repository.

Signed-off-by: Evgeny Vereshchagin <[email protected]>
jwcart2 pushed a commit to jwcart2/selinux that referenced this pull request Jan 6, 2022
@evverx @jwcart2
ci: run the tests under ASan/UBsan on GHActions
cafc084 
It was tested in SELinuxProject#321 and
SELinuxProject#320. In the process
it discovered a few issues all of which were fixed in
SELinuxProject@b98d3c4
SELinuxProject@ea53901
SELinuxProject@fe01a91
SELinuxProject@f95dbf2

Now that all the issues are gone it should be safe to turn it on
to make it easier to automatically catch bugs like that almost as soon as
they end up in the repository.

Signed-off-by: Evgeny Vereshchagin <[email protected]>
jwcart2 pushed a commit that referenced this pull request Jan 12, 2022
@evverx @jwcart2
ci: run the tests under ASan/UBsan on GHActions
ad2ff8a 
It was tested in #321 and
#320. In the process
it discovered a few issues all of which were fixed in
b98d3c4
ea53901
fe01a91
f95dbf2

Now that all the issues are gone it should be safe to turn it on
to make it easier to automatically catch bugs like that almost as soon as
they end up in the repository.

Signed-off-by: Evgeny Vereshchagin <[email protected]>
@cgzones cgzones force-pushed the sanitizer_test branch 2 times, most recently from f557dab to a89dad1 Compare July 12, 2022 16:17
chenyt9 pushed a commit to MotorolaMobilityLLC/external-selinux that referenced this pull request Mar 6, 2023
@evverx @jwcart2
ci: run the tests under ASan/UBsan on GHActions
df63830 
It was tested in SELinuxProject/selinux#321 and
SELinuxProject/selinux#320. In the process
it discovered a few issues all of which were fixed in
SELinuxProject/selinux@b98d3c4
SELinuxProject/selinux@ea53901
SELinuxProject/selinux@fe01a91
SELinuxProject/selinux@f95dbf2

Now that all the issues are gone it should be safe to turn it on
to make it easier to automatically catch bugs like that almost as soon as
they end up in the repository.

Signed-off-by: Evgeny VereshchaGin <[email protected]>
@cgzones cgzones changed the base branch from master to main November 1, 2023 17:08
@cgzones
libsemanage: free resources on failed connect attempt
7e6d842 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
ci: build all subdirectories in sanitizer job except python and sandbox
62d210a 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
ci: use -fsanitize-address-use-after-scope in sanitizer job
11ac995 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
Drop -z,defs linker flags
c7b38a4 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
ignore ebitmap math
fc4fcc3 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
libsemanage/tests: mark unused function parameters
38b9c9b 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
libsemanage: silence underflow
1439577 
Signed-off-by: Christian Göttsche <[email protected]>
@cgzones
libsemanage/tests: silence function UBSAN
10de147 
Signed-off-by: Christian Göttsche <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fishilico fishilico fishilico left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgzones @evverx @fishilico