This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add Strrplc() To Tapsets


On 06/16/2009 10:00 PM, Varun Chandramohan wrote:
> Ah, something i didnt notice. Thanks for that. Now, this makes me wonder,
> THIS->__retvalue max size would be MAXSTRINGLEN right? If so, if we have to 
> replace a sting which would result in a string much bigger than MAXSTRINGLEN 
> would cause an overflow right? Should we check that case too? Or am i missing 
> something?

All the strlcats are limiting the length of the complete result, not
just the number of bytes copied, which is why I prefer that over
strncat.  The return value of strlcat is the number of chars that it
wanted to copy, so you could check if that's >=MAXSTRINGLEN to see if
there was truncation, and perhaps throw an error.  I think it's ok to
silently truncate with no error though, as that's what we do everywhere
else.

A couple of comments on your new patches:

> + * prnt_str	 The parent string. If NULL, the function returns NULL.
> + *
> + * srch_str	 The substring which is used to search in the parent string
> + *		 prnt_str. If NULL, the function returns NULL.

I didn't notice this before, but the comments about NULL are meaningless
in our context.  I would mention that srch_str can't be an empty string
though.

> +	if(strlen_srch_str == 0) {
> +		CONTEXT->last_error,"Invalid Search String";
> +		return;
> +	}

Hmm, a comma doesn't assign anything here.  That should be an equal
sign, and then if someone attempts a bad string the script will abort
with an error.  Your testcase will need to be prepared to handle the
errors too, by setting -DMAXERRORS=<#-of-errors> and checking that the
output reports the errors.

> +	if((strlen_prnt_str - strlen_srch_str + strlen_rplc_str + 1) > MAXSTRINGLEN){
> +		strlcpy(THIS->__retvalue, THIS->prnt_str, MAXSTRINGLEN);
> +		return;
> +	}

You're trying to avoid truncation problems?  This only works if there's
exactly one replacement.  Besides, as I said, strlcat will be safe in
truncating the result, so I don't think you need this check.

> +	if(ptr_base == THIS->prnt_str) {
> +		strlcpy(THIS->__retvalue, THIS->prnt_str, MAXSTRINGLEN);
> +	} else {
> +		strlcat(THIS->__retvalue, ptr_base, MAXSTRINGLEN);
> +	}

Just the strlcat will suffice -- if ptr_base == prnt_str, then it will
strlcat onto an empty __retvalue, which is fine.

Thanks!

Josh


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]