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

isis: fix crash in isis_spf_process_lsp #10970

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+7 −2
Open

isis: fix crash in isis_spf_process_lsp #10970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions isisd/isis_spf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1213,11 +1213,16 @@ static int isis_spf_process_lsp(struct isis_spftree *spftree,
else
fragnode = listnextnode(fragnode);

if (fragnode) {
while (fragnode) {
lsp = listgetdata(fragnode);
goto lspfragloop;
if (lsp->tlvs)
Copy link
Contributor

Choose a reason for hiding this comment

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

so it looks as if this new test may terminate the iteration through the list of "fragnodes". is that ... correct?

Copy link
Contributor Author

@louis-6wind louis-6wind May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It was not. I have corrected that

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand this while loop will find the next non-empty fragment (i.e. with > 0 number of TLVs). And as soon as it finds it, breaks the loop and then goes back to the label 'lspfragloop' to process the fragment.

Like said before, I don't see a reason as to why will there be a fragment with zero TLVs in it in the first place. If this LSP/fragment is being originated by another instance of FRR code on a remote router, we need to fix the FRR ISIS LSP origination code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise these changes look good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand this while loop will find the next non-empty fragment (i.e. with > 0 number of TLVs). And as soon as it finds it, breaks the loop and then goes back to the label 'lspfragloop' to process the fragment.

Like said before, I don't see a reason as to why will there be a fragment with zero TLVs in it in the first place. If this LSP/fragment is being originated by another instance of FRR code on a remote router, we need to fix the FRR ISIS LSP origination code.

not originating from another FRR instance

break;
fragnode = listnextnode(fragnode);
}

if (fragnode)
goto lspfragloop;

return ISIS_OK;
}

Expand Down
Loading