Are these for safety reasons or have you found specific cases in other parts of code affected by not having this checks?

One comment I have is about the str dup functions, when the new check on len==0 results in setting dst s to NULL. Specs for malloc(0) are not very strict, saying that it can return a valid pointer for free(). However, the free(0) should not be done. With kamailio memory managers, pkg/shm malloc 0 returns a pointer of size(void*).

I think would be ok to set dst s to null if src s is null, but if src len is 0 and src s is not null, maybe is better to allocate it...

These are some thoughts, perhaps a deeper review where these str dup functions are used should be done.

--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/401#issuecomment-157680622

Considering the following _fallback case_ : - alloc dst->s = pointer sizeof(void*) - dst->len = 0 - return **0**

After skimming the code a little more I came to this for the above case(i.e. src->s is NULL): - either make dst->s = NULL; dst->len = 0 and return **-1**; this will be caught by shm_str_dup() != 0 and prevent shm_free() a NULL pointer, even though memory managers checks for this and gives L_WARN - or keep the fallback case; further uses of the dst should be done based on the dst->len (i.e. cmp_str() or LM_ERR("%.*s"))

I'd go for the second option. What do you think?

Also I've seen that the memory manager allocate and 'unsigned int'. So, if shm/pkg_str_dup() is called with src->len < 0 it will probably fail (depending on the negative value). In this case, I'd also force the fallback case.

--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/401#issuecomment-157984715

I guess there are no cases when one would try to duplicate src->s==NULL, or at least not an intentional case. Anyhow, printing a warning instead of a segfault is more friendly for runtime. You can merge if you prefer this version.

--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/401#issuecomment-158020344

Merged #401.

--- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/401#event-469176277