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

lib: fix some other Null Pointer Dereference bugs #18072

Closed
wants to merge 4 commits into from
Closed

lib: fix some other Null Pointer Dereference bugs #18072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ae79019
lib: fix a Null Pointer Dereference bug in mgmt_be_client.c
mugitya03 Feb 9, 2025
91a704c
lib: fix a Null Pointer Dereference bug in vrf.c
mugitya03 Feb 9, 2025
ad70a68
lib: fix a Null Pointer Dereference bug in pullwr.c
mugitya03 Feb 9, 2025
e2511de
lib: fix a Null Pointer Dereference bug in vrf.c
mugitya03 Feb 9, 2025
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
1 change: 1 addition & 0 deletions lib/mgmt_be_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ static int __send_notification(struct mgmt_be_client *client, const char *xpath,
int ret = 0;

assert(op != NOTIFY_OP_NOTIFICATION || xpath || tree);
assert(xpath || tree);
Copy link
Member

Choose a reason for hiding this comment

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

This looks odd at all. Why an assert is required here at all? I see it was introduced here opensourcerouting@c88b489, but maybe we can just fixup it correctly instead of adding a second assert?

Copy link
Member

Choose a reason for hiding this comment

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

Why does this commit not have the analysis of what is being done and it's in the PR? This makes no sense and I would like the commit to have this analysis. 5 years from now people are going to be looking at the code and commit messages not the PR

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. How about returning an error code when both xpath and tree are null instead of using an assert?
And I'll resubmit a PR with an analysis in the commit information.

debug_be_client("%s: sending %sYANG %snotification: %s", __func__,
op == NOTIFY_OP_DS_DELETE ? "delete "
: op == NOTIFY_OP_DS_REPLACE ? "replace "
Expand Down
2 changes: 1 addition & 1 deletion lib/pullwr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ static void pullwr_resize(struct pullwr *pullwr, size_t need)
}

niov = pullwr_iov(pullwr, iov);
if (niov >= 1) {
if (newbuf && niov >= 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This NULL check is superfluous. For newbuf to be NULL, pullwr->valid must be 0. When pullwr->valid is 0, pullwr_iov returns 0. Therefore niov will be 0 and the condition here is not met.

memcpy(newbuf, iov[0].iov_base, iov[0].iov_len);
if (niov >= 2)
memcpy(newbuf + iov[0].iov_len,
Expand Down
4 changes: 2 additions & 2 deletions lib/vrf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,8 +944,8 @@ static int lib_vrf_create(struct nb_cb_create_args *args)
return NB_OK;

vrfp = vrf_get(VRF_UNKNOWN, vrfname);

SET_FLAG(vrfp->status, VRF_CONFIGURED);
if (vrfp)
Copy link
Member

Choose a reason for hiding this comment

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

We should return a warning/error instead then? Also, check vrf_handler_create().

Copy link
Author

Choose a reason for hiding this comment

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

I think so. But I'm not sure which error code should be returned.

SET_FLAG(vrfp->status, VRF_CONFIGURED);
nb_running_set_entry(args->dnode, vrfp);

return NB_OK;
Expand Down
Loading