This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
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