BUG: maxfwd module incorrectly decrements header twice if the max-forwards…

[davidtrihy-genesys]

Summary
Solves an issue where using the maxfwd module function mf_process_maxfwd_header can incorrectly double decrement the header value if sipmsg_validate function with flag h is called before calling the aforementioned function

Details
This is a bug fix which should prevent future issues when using maxfwd which assumes that the parsed value in hdr_field will only ever be used by the maxfwds module which can cause an incorrect double decrement of the header value if some other module was the set the parsed memory to something other than null

Solution
The maxfwd module has two macros defined which store and fetch the value of the parsed property of the maxforwards header in the SIP Message, when storing it stores it adding 1 to the value and when fetching it subtracts 1 from the value. This is because this memory block is a void* and 0 represents a null pointer.

A problem arises when using sipmsg_validate to validate the headers, it will parse the value of the header into the parsed field and when maxfwds checks for the value it assumes it has already been added by the module which adds 1 to the value so when it fetches the value it will then subtract 1 from it causing the double decrement of the value.

The solution is a partial rewrite of the module where the addition and subtraction logic in the macro is removed in favour of assuming a value of 0 and a null value act exactly the same anyway so when the header is parsed and the value is 0 then we can just try and parse it from the body again anyway and end up with a value of 0 again but if the value is greater than 0 then we can just fetch the value in the parsed field and return, the trim logic which gets the string representation is called in all cases before checking if the value exists in the parsed header and/or fetching it. We can assume that if we get to trim stage then the header has a body at this stage

Compatibility
No compatibility issues, in fact it fixes a compatibility issue with sipmsgops and maxfwds

[davidtrihy-genesys]

Renamed the macros from MAXWD to MAXFWD also some of the changes here are whitespace changes to be consistent

[davidtrihy-genesys]

The else/if is removed here in favour of an if after this is so that we can do the trimlen in one place just before checking if IS_MAXFWD_STORED which it will be when using sipmsg_validate assuming it is non zero value so we trim and then check and retrieve the value

[davidtrihy-genesys]

Assume in this scenario that IS_MAXFWD_STORED failed because either we don't have a value in the parsed field because maxfwds module is the first instance of setting this value or that sipmsgops has not validated the SIP headers so we parse the body, we can only get here if the header has been parsed and if it is actually zero we can just return it

[github-actions]

Any updates here? No progress has been made in the last 30 days, marking as stale.

[bogdan-iancu]

Hi @davidtrihy-genesys , thanks for the spotting the issue and creating this PR.
Nevertheless, I see two easier approaches here :

1. the sipmsg_validate functions should not store (after checking) the parsed value for the MF header (it's not doing this for any other headers)
2. have the STORE_MAXWD_VAL and FETCH_MAXWD_VAL macros moved in core (in the parser dir somewhere) to be shared by all needing access to the header's value.

(1) is simple and more like putting the trash under the carpet, (2) is a cleaner approach IMHO. Do you see it as a feasible solution ?

[bogdan-iancu]

@davidtrihy-genesys , for the upcoming minor release (tomorrow) I pushed a quick fix/workaround, namely option (1). This is a temporary solution until completing the discussion here, for the final solution.

[davidtrihy-genesys]

Hi @bogdan-iancu

While I agree that sipmsg_validate should in reality not be storing the parsed value the issue in the macros would still exist whereby by calling retrieval macro still assumes you have to have stored using the other macro when direct access to parsed is not blocked in any way.

I understand the reasoning behind the original macro logic which was to differentiate between null pointer and an actual value of 0 but in the fix I put up for PR here it doesn't differentiate and if there is a header and it genuinely is 0 then it will just parse the string again and return that value and the rest of the same logic can run again. The increment is to solve one particular case where the Max-Forwards header is 0 when in reality that is not going to be case 99% of the time so the extra parse of the string in this scenario I feel is not a big issue.

We've been running with this fix in our branch since I opened this PR and there's been no issues and we have a set of multiple SIPP tests that run in various scenarios sipmsg_validate is called before this and sometimes not and our tests validate that the Max-Forwards is correct, in fact those tests are what notified us of this issue.

I don't object to a different fix than what I have presented here but I do feel strongly about the macros incrementing on storage of the value and decrementing retrieval of the value being an incorrect way to solve this issue. Let me know your thoughts and I'd be happy to work with you and run some SIPP tests.