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

API: Check optlen for string OPTIONS (e.g. SRTO_STREAMID). #2845

Closed
2 tasks done
maxsharabayko opened this issue Jan 8, 2024 · 7 comments
Closed
2 tasks done

API: Check optlen for string OPTIONS (e.g. SRTO_STREAMID). #2845

maxsharabayko opened this issue Jan 8, 2024 · 7 comments
Assignees
Labels
[API] Area: Changes in SRT library API Type: Maintenance Work required to maintain or clean up the code
Milestone

Comments

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Jan 8, 2024

Bug Description

Negative option lengths are not checked in srt_setsockopt() calls, except for those having integer types.

A memcpy with the to unsigned converted negative value is being done then.

To Reproduce

srt_setsockopt(SRTO_STREAMID, ..., -1 /*optlen*/);

Expected behavior

Report SRT_EINVPARAM.

TODO

  • Handle invalid optlen values where not handled.
  • Add a unit test to validate the expected behavior.
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [API] Area: Changes in SRT library API labels Jan 8, 2024
@maxsharabayko maxsharabayko added this to the v1.5.4 milestone Jan 8, 2024
@maxsharabayko maxsharabayko changed the title [BUG] API: Check optlen for string OPTIONS (e.g. SRTO_STREAMID). API: Check optlen for string OPTIONS (e.g. SRTO_STREAMID). Jan 8, 2024
@yomnes0 yomnes0 self-assigned this Jan 8, 2024
yomnes0 added a commit to yomnes0/srt that referenced this issue Jan 9, 2024
yomnes0 added a commit to yomnes0/srt that referenced this issue Jan 9, 2024
@yomnes0
Copy link
Collaborator

yomnes0 commented Jan 9, 2024

PR #2849 should address this by returning SRT_EINVPARAM whenever optlen is set to -1

@ethouris
Copy link
Collaborator

ethouris commented Jan 9, 2024

Wait. What was the final decision then?

Note that there are already applications that RELY on that setting a string-typed option with passing -1 as length treats the string as a NUL-terminated array of characters.

@maxsharabayko
Copy link
Collaborator Author

maxsharabayko commented Jan 9, 2024

The majority has voted for not using optlen=-1 (7 votes against using, only 2 support).
The "feature" is only implemented for SRTO_BINDTODEVICE and SRTO_CONGESTION and not documented officially. Thus may be considered as a bug and safely fixed without breaking the API/ABI backward compatibility.

@ethouris
Copy link
Collaborator

ethouris commented Jan 9, 2024

Ah, in that case, it should be ok.

@jlsantiago0
Copy link
Contributor

Perhaps using -1 or size can also help solve #2284

@maxsharabayko
Copy link
Collaborator Author

@jlsantiago0
#2284 has been already fixed by #2804.

@maxsharabayko
Copy link
Collaborator Author

Fixed by #2849.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[API] Area: Changes in SRT library API Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

No branches or pull requests

4 participants