[PATCH phosphor-event v4 2/2] Fixed Multiple associations occasionally geting truncated

[Cyril Bur]

On Fri, 11 Mar 2016 16:40:30 -0600
OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:

> From: Chris Austen <austenc at us.ibm.com>
> 
> After additional testing it was found that the association list gets truncated
> leaving you with only one item.  Upon code review it was realized that the
> use of strtok manipulated a cached string and tokenized the string with NULLs.
> 
> Changed to use a temp buffer for strtok to manipulate and switched to
> strtok_r since it is a best practice to ensure things are thread safe.

Oh ok, I get v4 now. Since you introduce this code in the previous patch, these
fixes should be squashed into the previous patch. Reason for this is that each
patch should leave the repository in a state where it works. Of course as it
often happens bugs get found after code is added, this is the reason for fix
patches.

And as stated in my response to 1/2, this is a perfect candidate for the
changelog I was talking about.

Few problems with the patch though.

> ---
>  event_messaged_sdbus.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/event_messaged_sdbus.c b/event_messaged_sdbus.c
> index 907ac53..ff9fd5e 100644
> --- a/event_messaged_sdbus.c
> +++ b/event_messaged_sdbus.c
> @@ -99,7 +99,7 @@ static int prop_message_assoc(sd_bus *bus,
>  	messageEntry_t *m = (messageEntry_t*) userdata;
>  	event_record_t *rec;
>  	char *p;
> -	char *token;
> +	char *token, *saveptr;
>  
>  	rec = message_record_open(m->em, m->logid);
>  	if (!rec) {
> @@ -110,9 +110,13 @@ static int prop_message_assoc(sd_bus *bus,
>  		return -1;
>  	}
>  
> -	p = rec->association;
> +	/* strtok manipulates a string.  That means if any caching was done   */
> +	/* then the original string would get messed up.  To avoid that since */
> +	/* I know there is caching done simple make a copy and mess with that */

I think I see what you're trying to say here but the word caching is... odd.

> +	asprintf(&p, "%s", rec->association);

How about:
p = strdup(rec->association);

It was designed for your use case :).

Few other notes, should probably check the return value of either strdup or
asprintf to make sure p was actually allocated.

In the case of using strdup it might not be disastrous if strtok handles being
passed NULL (I worry it won't though), however, in the case of asprintf, since
it doesn't say anything about the value of p if it returns -1 then bad things
are likely to happen upon passing that to strtok.

Also, can you know the maximal possible size of association here? I would feel
safer using strndup().

One more thing, why did you change to using strtok_r()? According to the
manpage, they do the same thing except that strtok() isn't thread safe, do you
care though, this isn't multi threaded in any way is it?

Thanks,

Cyril

> +
> +	token = strtok_r(p, " ", &saveptr);
>  
> -	token = strtok(p, " ");
>  	if (token) {
>  		r = sd_bus_message_open_container(reply, 'a', "(sss)");
>  		if (r < 0) {
> @@ -120,18 +124,19 @@ static int prop_message_assoc(sd_bus *bus,
>  		}
>  
>  		while(token) {
> -
>  			r = sd_bus_message_append(reply, "(sss)", "fru", "event", token);
>  			if (r < 0) {
>  				fprintf(stderr,"Error adding properties for %s to reply %s\n", token, strerror(-r));
>  			}
>  
> -			token = strtok(NULL, " ");
> +			token = strtok_r(NULL, " ", &saveptr);
>  		}
>  
>  		r = sd_bus_message_close_container(reply);
>  	}
>  
> +	free(p);
> +
>  	return r;
>  }
>