[PATCH] [CTDB] CID 1435726: NULL pointer dereference

[Martin Schwenke]

Hi Swen,

On Fri, 25 May 2018 09:34:15 +0200, Swen Schillig via samba-technical
<samba-technical at lists.samba.org> wrote:

> Please review and push if happy.

>  	if (mode == CONF_MODE_RELOAD) {
> -		if (strcmp(old_value, new_value) != 0) {
> +		if (!(old_value && new_value) ||
> +		    (strcmp(old_value, new_value) != 0)) {
>  			D_WARNING("Ignoring update of [%s] -> %s\n",
>  				  CLUSTER_CONF_SECTION,
>  				  key);

Won't this generate a warning about ignoring an update if both pointers
are NULL (and, therefore, equal)?  Sadly, I think the extra check
probably needs to be expanded quite a lot to make it logically correct.

Also, although README.Coding doesn't seem to mention it, Samba
seems to prefer "old_value != NULL" instead of just "old_value".

Finally, the bizarre thing about this particular Coverity finding is
that I don't think there's actually a way for new_value to be NULL
*during a reload*.  However, Coverity takes new_node_address == NULL to
indicate that it can generally be NULL.

And even more finally, the code should actually be warning about
updates to relevant parameters when the API is used (e.g.
conf_set_string()), in which case this can definitely be NULL.

Yep, I certainly didn't think about all of the corner cases when
writing that code...  ;-)

peace & happiness,
martin