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

Clear next field when returnining list elements in queue.c #2283

Closed
wants to merge 2 commits into from

Conversation

frozencemetery
Copy link
Contributor

@frozencemetery frozencemetery commented Aug 24, 2018

The ipa-otpd code occasionally removes elements from one queue,
inspects and modifies them, and then inserts them into
another (possibly identical, possibly different) queue. When the next
pointer isn't cleared, this can result in element membership in both
queues, leading to double frees, or even self-referential elements,
causing infinite loops at traversal time.

Rather than eliminating the pattern, make it safe by clearing the next
field any time an element enters or exits a queue.

Related https://pagure.io/freeipa/issue/7262

@Tiboris Tiboris added the ipa-next Mark as master (4.12) only label Aug 27, 2018
@Tiboris Tiboris added the needs review Pull Request is waiting for a review label Aug 27, 2018
@Tiboris
Copy link
Member

Tiboris commented Aug 28, 2018

Could you please add line:
Related https://pagure.io/freeipa/issue/7262
into commit message?

@Tiboris Tiboris added ipa-4-7 ipa-4-6 Mark for backport to ipa 4.6 and removed ipa-next Mark as master (4.12) only labels Aug 28, 2018
@Tiboris
Copy link
Member

Tiboris commented Aug 28, 2018

@frozencemetery nevermind I did it myself :)

@spoore1
Copy link
Contributor

spoore1 commented Aug 29, 2018

FYI I tested a 4.6 version of this and it seems to fix the crash. I've run tests similar to the pagure 7262 description for many hours (12 last night, multiple tests before). I've seen no new crashes since applying this patch.

@rcritten
Copy link
Contributor

It would be really great to have a unit test to test the queueing code.

Copy link
Contributor

@flo-renaud flo-renaud left a comment

Choose a reason for hiding this comment

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

Hi,
please find an inline comment.

@frozencemetery
Copy link
Contributor Author

Also added unit tests.

@Tiboris
Copy link
Member

Tiboris commented Aug 31, 2018

Hello @frozencemetery,
Thank you for the patch!
Sorry for nitpicking, unfortunately line with ticket number in commit message disappeared.
Could you please add it?

@flo-renaud
Copy link
Contributor

Hi @frozencemetery
thank you for the patch and the tests, works for me. Waiting for the updated commit msg with a ref to the ticket, then I will ACK.

The ipa-otpd code occasionally removes elements from one queue,
inspects and modifies them, and then inserts them into
another (possibly identical, possibly different) queue.  When the next
pointer isn't cleared, this can result in element membership in both
queues, leading to double frees, or even self-referential elements,
causing infinite loops at traversal time.

Rather than eliminating the pattern, make it safe by clearing the next
field any time an element enters or exits a queue.

Related https://pagure.io/freeipa/issue/7262
@frozencemetery
Copy link
Contributor Author

Heh, sorry @Tiboris! Fixed now.

@flo-renaud flo-renaud added ack Pull Request approved, can be merged pushed Pull Request has already been pushed and removed needs review Pull Request is waiting for a review labels Aug 31, 2018
@flo-renaud
Copy link
Contributor

master:

  • fe65008 Clear next field when returnining list elements in queue.c
  • ab63668 Add cmocka unit tests for ipa otpd queue code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-6 Mark for backport to ipa 4.6 pushed Pull Request has already been pushed
Projects
None yet
5 participants