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

Fix potential null pointer deferenced #363

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Conversation

raniervf
Copy link
Contributor

clist_content is a MACRO and return NULL:
#define clist_content(iter) (iter ? (iter)->data : NULL)

clist_content is a MACRO and return NULL:
#define     clist_content(iter)            (iter ? (iter)->data : NULL)
Copy link
Owner

@dinhvh dinhvh left a comment

Choose a reason for hiding this comment

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

I'm pretty sure we don't need those changes (or we need to formulate them differently to make the compiler happy)

@@ -1716,7 +1716,7 @@ static int imap_mailbox_list_to_group(clist * imap_mb_list, clistiter ** iter,
imap_mailbox_listiter = * iter;

imap_addr = clist_content(imap_mailbox_listiter);
if (imap_addr->ad_mailbox_name == NULL) {
if (imap_addr != NULL && imap_addr->ad_mailbox_name == NULL) {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure having imap_addr == NULL is not a valid use case.
(and it would crash on lines below)

So, there's two options:
Either we test at the beginning of the function and return MAIL_ERROR_INVAL or maybe the code is fine after all and imap_mailbox_list_to_group() is used only with the content of iter not NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you right, better test is:
if (imap_addr == NULL || imap_addr->ad_mailbox_name == NULL) {
What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Perfect.

@@ -1739,7 +1739,7 @@ static int imap_mailbox_list_to_group(clist * imap_mb_list, clistiter ** iter,

imap_addr = clist_content(cur);

if (imap_addr->ad_mailbox_name == NULL) {
if (imap_addr != NULL && imap_addr->ad_mailbox_name == NULL) {
Copy link
Owner

Choose a reason for hiding this comment

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

Here, cur is guaranteed to be non null via the for loop, so we don't need to do that check.
The only option left is imap_mailbox_listiter containing NULL in the list of mailbox it has.
It probably means that invalid data has been input to that function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@dinhvh dinhvh merged commit 68bde8b into dinhvh:master Apr 30, 2020
dsanghan pushed a commit to canarymail/libetpan that referenced this pull request Sep 6, 2022
* Fix potential null pointer deferenced

clist_content is a MACRO and return NULL:
#define     clist_content(iter)            (iter ? (iter)->data : NULL)

* FIx invalid test against NULL.
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.

2 participants