lib: fix some other Null Pointer Dereference bugs

[mugitya03]

Dear Developers,

We found and fixed some other potential Null Pointer Dereference bugs similar to 18067. Here are the details:

1
Function mgmt_be_send_ds_delete_notification passes a null value as the third argument to function __send_notification at line 371.

return __send_notification(__be_client, path, patch, NOTIFY_OP_DS_PATCH);

Then, in function __send_notification, when both the 2nd parameter xpath and 3rd parameter tree are null, a null pointer dereference bug occurs at line 331.

	assert(op != NOTIFY_OP_NOTIFICATION || xpath || tree);
	debug_be_client("%s: sending %sYANG %snotification: %s", __func__,
			op == NOTIFY_OP_DS_DELETE    ? "delete "
			: op == NOTIFY_OP_DS_REPLACE ? "replace "
			: op == NOTIFY_OP_DS_PATCH   ? "patch "
						     : "",
			op == NOTIFY_OP_NOTIFICATION ? "" : "DS ", xpath ?: tree->schema->name);

To prevent this, we have added a check to ensure that xpath and tree are not both null simultaneously.

2
Function vrf_get can return NULL value at line 148.

	if (vrf && vrf_id != VRF_UNKNOWN
	    && vrf->vrf_id != VRF_UNKNOWN
	    && vrf->vrf_id != vrf_id) {
		zlog_debug("VRF_GET: avoid %s creation(%u), same name exists (%u)",
			   name, vrf_id, vrf->vrf_id);
		return NULL;
	}

Then in function lib_vrf_create, the return value from vrf_get is dereferenced at line 948 without null value check, causing a null pointer dereference bug. We noticed that other caller functions both check the return value before dereferencing it (link), thus we add the null value check in function lib_vrf_create.

3

Function pullwr_resize, when need is false and pullwr->valid is false, the pointer newbuf is set to NULL at line (130)[https://github.com/FRRouting/frr/blob/2ef76a33506f39d98038d5239f0620e2bbf33d8c/lib/pullwr.c#L130].

	} else if (!pullwr->valid) {
		/* resize down, buffer empty */
		newsize = 0;
		newbuf = NULL;
	}

Then the pointer newbuf is passed as the first argument to function memcpy, causing a null pointer dereference bug.

	niov = pullwr_iov(pullwr, iov);
	if (niov >= 1) {
		memcpy(newbuf, iov[0].iov_base, iov[0].iov_len);
		if (niov >= 2)
			memcpy(newbuf + iov[0].iov_len,
				iov[1].iov_base, iov[1].iov_len);
	}

Thus, we add a null value check before calling the function memcpy.

[ton31337]

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?

[donaldsharp]

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

[mugitya03]

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.

[ton31337]

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

[mugitya03]

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

[ton31337]

Remove this commit at all.

[donaldsharp]

Please put the analysis in the commits themselves until then this is a nack from me

[eqvinox]

The pullwr change is superfluous.

[eqvinox]

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.