Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add strlcat variant that uses memccpy for writing SDPs #2835

Merged
merged 2 commits into from
Dec 15, 2021
Merged

Conversation

lminiero
Copy link
Member

We recently found out that the process of creating/writing SDPs could be slow, due to how g_strlcat works. This is an attempt to speed that part up by adding a janus_strlcat variant that uses memccpy instead of g_strlcat. It seems to be working as expected in some tests when using SDP fuzzing with a huge (~20k mlines) SDP, as it's basically 10x faster:

[lminiero@lminiero janus-gateway]$ time ./fuzzers/out/sdp_fuzzer ~/Downloads/giganto.sdp 
Running: /home/lminiero/Downloads/giganto.sdp
Done:    /home/lminiero/Downloads/giganto.sdp: (410058 bytes)

real	0m3.556s
user	0m3.523s
sys	0m0.022s
[lminiero@lminiero memccpy]$ time ./fuzzers/out/sdp_fuzzer ~/Downloads/giganto.sdp 
Running: /home/lminiero/Downloads/giganto.sdp
Done:    /home/lminiero/Downloads/giganto.sdp: (410058 bytes)

real	0m0.340s
user	0m0.324s
sys	0m0.016s

and a simple test with the EchoTest demo worked flawlessly too, but of course we need to ensure this doesn't break anything in production, hence why this is still a PR and not in master yet, so feedback welcome! Once this is confirmed to be ok, I'll update the multistream branch as well where this enhancement would definitely have more of an impact.

utils.c Outdated
}
char *p = memccpy(dest + *offset, src, 0, dest_size - *offset);
if(p == NULL) {
JANUS_LOG(LOG_ERR, "janus_strlcat: truncation occurred, %lu >= %lu\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: to avoid confusion, probably better to log "janus_strlcat_fast:...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a copy/paste typo, thanks for catching! Incidentally, since this is a LOG_ERR, I think the name of the function is actually redundant, since it will appear in the prefix anyway, e.g.:

[ERR] [utils.c:janus_strlcat_fast:288] janus_strlcat_fast: overflow

so I'll just remove it from both methods.

@lminiero
Copy link
Member Author

Merging.

@lminiero lminiero merged commit fc9ece3 into master Dec 15, 2021
@lminiero lminiero deleted the memccpy branch December 15, 2021 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants