<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> Fix invalid memory access (beyond the string). You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2512
-- Commit Summary --
* janssonrpcc: Zero-terminate all string copies * janssonrpcc: Remove unused variable and redundant assignment
-- File Changes --
M src/modules/janssonrpcc/janssonrpc.h (32) M src/modules/janssonrpcc/janssonrpc_funcs.c (15) M src/modules/janssonrpcc/janssonrpc_server.c (14) M src/modules/janssonrpcc/janssonrpc_srv.c (12)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2512.patch https://github.com/kamailio/kamailio/pull/2512.diff
Sorry about the regression, the problem is that "str" is not supposed to be null terminated. Its probably better to use "STR_VTOZ" instead of adding another str_dup implementation.
I would rather change pkg_str_dup() and shm_str_dup() to allocate len+1 and add the ending zero, right now there are not widely used and at the end is just adding an extra operation `newbuf->s[len] = '\0';`. IMO it will be better and safer in long term, as one can then use standard string operations with newbuf->s. For that reason, over the years the values for variables (like avp, xavp, ...) were made zero-terminated.
STR_VTOZ macro is useful when one knows that the string is stored inside a larger buffer, so temporarily setting an ending 0 doesn't go over allocated memory, otherwise can end up in segmentation fault.
I would rather change pkg_str_dup() and shm_str_dup() to allocate len+1 and add the ending zero
That was my first approach. I just thought it would provoke more discussion (see below) and I wanted to fix this bug fast, so I just kinda reverted the commits that introduced it.
The problem with adding a zero on every copy is that a ```str``` is supposed to be useful without a terminating zero. Lots of ```str``` uses are within the parser where ```str``` just points into the message buffer and has no terminating 0. Having them mixed will inevitable lead to bugs (just like the one I'm trying to fix). All developers will have to remember that only copies are 0-terminated.
Introducing a separate type for it (```strz```) will need duplicates for all existing str functions as C doesn't allow type inheritance.
Another option is to create ```pkg_str_dupz()``` and ```shm_str_dupz()``` which will 0-terminate the copies.
The `len` field will not include the 0, so copying such a cloned value will not be affected by ending 0. Besides the str fields that point in larger buffers (like with the sip parser fields that point inside msg->buf), most of the other str->s values are 0 terminating (e.g., most of pseudo-variables values, functions parameters evaluated by fixup_get_svalue(), ...). I am sure there are way more 0-terminated str->s fields than the non-0-terminated.
Again, here we discuss about cloning values, their result is not impacting anything else, they are supposed to be independent of other resources.
The rule is more like: do not rely on 0-terminated str->s value unless you are sure it is.
In this case it is made sure to be 0-terminated and used accordingly.
Thanks, I will squash and merge.
Merged #2512 into master.
-
Alex Hermann -
Daniel-Constantin Mierla
-
Henning Westerholt