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

Convert strlen calls to strnlen #3059

Merged
merged 7 commits into from
Sep 9, 2022
Merged

Conversation

tmatth
Copy link
Contributor

@tmatth tmatth commented Sep 8, 2022

  • For strlen(x) > c comparisons, use strnlen(x, c + 1) > c (so O(1) instead of O(n))
  • For strlen(x) < c comparisons, use strnlen(x, c) < c, so (so O(1) instead of O(n))

I didn't change strlen(x) > 0 comparisons (or any other comparison to 0) since compilers already optimize away the strlen call by transforming those to e.g., x[0] != '\0', see https://godbolt.org/z/KGW359ca1

N.B. Some of these calls are still followed by strlen calls, but this is always in the error and/or logging case (not the most likely path).

Inspired by https://code.videolan.org/videolan/vlc/-/merge_requests/2477/diffs

@lminiero
Copy link
Member

lminiero commented Sep 8, 2022

I wasn't aware of this function, thanks! Is it portable enough as strlen is, or do we risk this breaking compilation in some places where it now works? Unfortunately it doesn't look like there's a g_strnlen we can piggyback to 🙂

@tmatth
Copy link
Contributor Author

tmatth commented Sep 8, 2022

I wasn't aware of this function, thanks! Is it portable enough as strlen is, or do we risk this breaking compilation in some places where it now works? Unfortunately it doesn't look like there's a g_strnlen we can piggyback to slightly_smiling_face

It's required for POSIX so I think officially supported platforms are ok.

@atoppi
Copy link
Member

atoppi commented Sep 8, 2022

Always cool stuff from @tmatth 😎
Kudos for the online service compiling the code in real time with different compilers and flags.
That's brilliant.

I can see the enhancement here, but being pedantic

For strlen(x) > c comparisons, use strnlen(x, c + 1) > c (so O(1) instead of O(n))

AFAIU that will not be O(1), but O(c+1).
It would theoretically be O(1) if C had a string representation like e.g. Golang, with a "length" attribute.

@tmatth
Copy link
Contributor Author

tmatth commented Sep 8, 2022

AFAIU that will not be O(1), but O(c+1). It would theoretically be O(1) if C had a string representation like e.g. Golang, with a "length" attribute.

Where c is a constant that is bounded by a value that does not depend on the size of the input it simplifies to O(1).

But you're right that it will take up to c+1 operations 😆

@atoppi
Copy link
Member

atoppi commented Sep 8, 2022

mh looks like strnlen has been excluded from C23 standard, I'm wondering if that might be a problem in future

@tmatth
Copy link
Contributor Author

tmatth commented Sep 8, 2022

mh looks like strnlen has been excluded from C23 standard, I'm wondering if that might be a problem in future

That's an interesting find. I think it's ok since it's already not part of the standard, but won't be dropped from POSIX. If it does at some point become a problem it'd be trivial to add a fallback using memchr (but I wouldn't use memchr directly since it's a bit less readable).

@lminiero
Copy link
Member

lminiero commented Sep 9, 2022

Thanks for the discussion and the contribution, merging then!

@lminiero lminiero merged commit 11247a6 into meetecho:master Sep 9, 2022
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Oct 4, 2022
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.0.4` -> `v1.1.0` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway</summary>

### [`v1.1.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v110---2022-10-03)

[Compare Source](meetecho/janus-gateway@v1.0.4...v1.1.0)

-   Added versioning to .so files \[[PR-3075](meetecho/janus-gateway#3075)]
-   Allow plugins to specify msid in SDPs \[[PR-2998](meetecho/janus-gateway#2998)]
-   Fixed broken RTCP timestamp on 32bit architectures \[[Issue-3045](meetecho/janus-gateway#3045)]
-   Fixed problems compiling against recent versions of libwebsockets \[[Issue-3039](meetecho/janus-gateway#3039)]
-   Updated deprecated DTLS functions to OpenSSL v3.0 [PR-3048](meetecho/janus-gateway#3048)]
-   Switched to SHA256 for signing self signed DTLS certificates (thanks [@&#8203;tgabi333](https://github.com/tgabi333)!) \[[PR-3069](meetecho/janus-gateway#3069)]
-   Started using strnlen to optimize performance of some strlen calls (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3059](meetecho/janus-gateway#3059)]
-   Added checks to avoid RTX payload type collisions \[[PR-3080](meetecho/janus-gateway#3080)]
-   Added new APIs for cascading VideoRoom publishers \[[PR-3014](meetecho/janus-gateway#3014)]
-   Fixed deadlock when using legacy switch in VideoRoom \[[Issue-3066](meetecho/janus-gateway#3066)]
-   Fixed disabled property not being advertized to subscribers when VideoRoom publishers removed tracks
-   Fixed occasional deadlock when using G.711 in the AudioBridge \[[Issue-3062](meetecho/janus-gateway#3062)]
-   Added new way of capturing devices/tracks in janus.js \[[PR-3003](meetecho/janus-gateway#3003)]
-   Removed call to .stop() for remote tracks in demos \[[PR-3056](meetecho/janus-gateway#3056)]
-   Fixed missing message/info/transfer buttons in SIP demo page
-   Fixed postprocessing compilation issue on older FFmpeg versions \[[PR-3064](meetecho/janus-gateway#3064)]
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMTMuMCIsInVwZGF0ZWRJblZlciI6IjMyLjIxMy4wIn0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/91
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
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.

None yet

3 participants