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

Fixing srt_getsockopt for bool socket options #1925

Merged
merged 5 commits into from
Apr 14, 2021

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Apr 8, 2021

In srt_getsockopt(..) API function these bool socket options were actually using int type when casting the optval, which may lead to writing outsize the memory of the optval when bool is provided.

  • SRTO_SENDER
  • SRTO_TSBPDMODE
  • SRTO_DRIFTTRACER
  • SRTO_ENFORCEDENCRYPTION

This PR changes types to bool if optlen is zero of sizeof(bool), otherwise casts to int.

ABI Change

Fixing cast to bool is the lesser evil compared to potential outside-of-memory writing.
This change however affects SRT API a bit and might lead to certain upgrading difficulties.
Both FFMpeg and GStreamer should not get into trouble with this change, as they only get the value of SRTO_PAYLOADSIZE.

Still, the output optlen should provide a hint of how many bytes were actually set by srt_getsockopt(..) function.

Note that the type used to set those bool options is still int there, as both int and bool types are supported in srt_setsockopt(..).

TODO

  • Consider if this breaks ABI compatibility with previous SRT versions
  • Check srt_getopt for SRTO_TLPKTDROP: does not use the value from config (to be addressed separately).

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Apr 8, 2021
@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Apr 8, 2021
if optlen is not provided or is sizeof(bool), then sets bool, otherwise assigns int
srtcore/socketconfig.h Outdated Show resolved Hide resolved
srtcore/core.cpp Show resolved Hide resolved
srtcore/socketconfig.h Outdated Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator Author

Reverted the second commit, which was taking the optlen value into account. ☝️

@maxsharabayko
Copy link
Collaborator Author

Related documentation. PR #1941: add an instruction to provide optval size in optlen when calling srt_getsockopt(..).

@maxsharabayko maxsharabayko marked this pull request as ready for review April 14, 2021 10:42
@maxsharabayko maxsharabayko merged commit 4f06c2e into Haivision:master Apr 14, 2021
@maxsharabayko maxsharabayko deleted the hotfix/sockopt_get_bool branch April 14, 2021 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants