Issue 4180 heap use after free cloned hdr

[ablangy]

This pull request should fix the issue 4180

[sauwming]

This failed the CI tests

[nanangizz]

Just realized that it is a generic header, using pjsip_generic_int_hdr_create() will always reset the header type to PJSIP_H_OTHER. So from the original code, only adding header name duplication (especially if the type is PJSIP_H_OTHER) is sufficient?

[ablangy]

Hello,

pjsip_generic_int_hdr_create() calls pjsip_generic_int_hdr_init() which will allocate new space in the provided pj_pool_t for the header name of the cloned header via pj_strdup() (pjsip/src/pjsip/sip_msg.c:863), this is what we need.
Otherwise, the header name of the cloned header points to a memory space of the source header pj_pool_t.
I think this is sufficient for the error detected by ASAN.

[nanangizz]

Hi,

As mentioned before, the problem with the patch is that pjsip_generic_array_hdr_create()/pjsip_generic_int_hdr_create() will reset the header type to PJSIP_H_OTHER. Note that the cloned header can be pjsip_expires_hdr which is derived from pjsip_generic_int_hdr and has header type of PJSIP_H_EXPIRES.

So if the missing part is only pj_strdup() for the header name (especially for PJSIP_H_OTHER), why not just add only it, instead of replacing the whole thing with pjsip_generic_*_hdr_create() (which may reset things such as header type).

[nanangizz]

Similar to above.

[ablangy]

pjsip_generic_array_hdr_create() does the same by calling pjsip_generic_array_hdr_init() which allocate new space in the provided pj_pool_t for the header name of the cloned header via pj_strdup() (pjsip/src/pjsip/sip_msg.c:939).
The problem doesn't exist with header which are not PJSIP_H_OTHER because the header name points to a static memory space defined by const pjsip_hdr_name_info_t pjsip_hdr_names[];.

Found a potential issue

[ablangy]

Alright, I didn't get your point, now it's clear, you are right.
This correction should do the job the same way the other "clone" functions do, but I missed the header type point.
If I keep the code as it was and just add the missing pj_strdup(), then the cloned header list pointers still point to the source and not to the cloned header, which can also be an issue.
A deep clone function shall create a new header and then copy the specific informations from the source, that way we do not have to be aware of the content of the 'pjsip_hdr' structure which can evolved with less impact on the existing code. This is what is more or less done in the other "clone" functions in the sip_msg.c file.
I propose a solution close to the one applied to pjsip_generic_string_hdr, if you think it is not apropriate, I will restore the initial pj_memcpy() and set the cloned header type in the clone functions.

[nanangizz]

If I keep the code as it was and just add the missing pj_strdup(), then the cloned header list pointers still point to the source and not to the cloned header, which can also be an issue.

You're right.

A deep clone function shall create a new header and then copy the specific informations from the source, that way we do not have to be aware of the content of the 'pjsip_hdr' structure which can evolved with less impact on the existing code. This is what is more or less done in the other "clone" functions in the sip_msg.c file. I propose a solution close to the one applied to pjsip_generic_string_hdr, if you think it is not apropriate, I will restore the initial pj_memcpy() and set the cloned header type in the clone functions.

You're right again.

Just perhaps creating pjsip_generic_int_hdr_init2()+create2() may not sound ideal because generic is basically corresponding to PJSIP_H_OTHER (including in pjsip_generic_string_hdr) and now it will be variable/settable, and the naming gives an impression like overloading existing APIs while they are actually internal function helpers. IMO using the existing pjsip_generic_int_hdr_create() and overriding the header type should be sufficient and acceptable?

Also, just a minor, it is better to maintain the column width limit <= 80.

[ablangy]

Alright, I have removed pjsip_generic_int_hdr_init2()+create2() and override the header type directly in the clone functions.

[ablangy]

hdr->count = PJ_MIN(rhs->count, PJSIP_GENERIC_ARRAY_MAX_COUNT) added