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

video emergency hotfix #622

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
7 participants
@zoff99

zoff99 commented Nov 24, 2017

to get at least 65k bytes (and not less) out of those crippled packets, apply this emergency fix to 0.1.10


This change is Reviewable

@zoff99 zoff99 requested review from nurupo and mannol Nov 24, 2017

@CLAassistant

This comment has been minimized.

CLAassistant commented Nov 24, 2017

CLA assistant check
All committers have signed the CLA.

@zoff99

This comment has been minimized.

zoff99 commented Nov 24, 2017

please squash ("Allow edits from maintainers" is on)

@nurupo nurupo changed the title from video emergency hotfix to Video fix (non-backwards compatible change, breaks protocol) Nov 24, 2017

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Removed the "emergency" part and added text that it breaks protocol. "Emergency" implies that it should be merged asap, but we can't really do that without any planning as this would break the protocol. We might even have to wait out a release or two before merging this change, depending on what we decide to do with it.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Btw, I know I have already commented on it months ago, but your git workflow is horrible :\ All these merge commits of PRs with c-toxcore changes, when you could have just rebased your repo on TokTok/c-toxcore's master without any extra merge commits. Not sure if this is manageable, haven't dealt with such PRs before, someone might have to manually rewrite your fixes and create a new clean PR with them.

@SkyzohKey

This comment has been minimized.

SkyzohKey commented Nov 24, 2017

@nurupo Just squash it and it should be fine, nope? As only one file changed

@SkyzohKey SkyzohKey added this to Triage in Video via automation Nov 24, 2017

@SkyzohKey SkyzohKey moved this from Triage to In Progress in Video Nov 24, 2017

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Hm, in the issue #620 you changed struct members, but in here you just bound some values? What is the purpose of bounding them?

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Also, why number 65533? Why not 65535, for example? I'm not familiar with toxav code, so I might be missing something obvious.

@zoff99 zoff99 changed the title from Video fix (non-backwards compatible change, breaks protocol) to video emergency hotfix Nov 24, 2017

@zoff99

This comment has been minimized.

zoff99 commented Nov 24, 2017

@nurupo
please first understand, read. and then change stuff.

this is the emergency hotfix, that does NOT break protocol!
65535 will not work here. its at least 1 less. but to be sure i made it 2 less.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Yeah, sorry, I assumed it was the same fix you linked in the issue without looking at the code.

@zoff99

This comment has been minimized.

zoff99 commented Nov 24, 2017

this could be released (after testing) quickly as 0.10.1 hotfix. so that we at least salvage what we have now out there

@mannol

This comment has been minimized.

mannol commented Nov 24, 2017

I recommend trying this first, before making such a big change.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

Reviewed 1 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toxav/toxav.c, line 721 at r3 (raw file):

        }

		uint32_t frame_size_safe = vrc + sizeof(sampling_rate);

Let's do bound checking to make sure no overflow can occur here.

  • vrc is of type int, meaning it's at least 16 bits.
  • vrc can't be negative, we checked this above.
  • vrc is signed, so it has only half of its positive range.
  • sizeof(sampling_rate) is 4.

There might be an issue if int is 64-bit or greater. A signed 64-bit integer can go as far as (2^63)-1, while uint32_t can hold at most (2^32)-1, meaning that frame_size_safe might overflow due to possible type narrowing.

The safe version of this code would be to do

#include <stdint.h>

// make sure vrc + sizeof(sampling_rate) <= UINT16_MAX - 2
uint16_t frame_size_safe;
if (vrc <= UINT16_MAX - 2 - sizeof(sampling_rate)) {
    frame_size_safe = vrc;
} else {
    frame_size_safe = UINT16_MAX - 2;
}

or with casts, promoting non-negative int vrc to unsigned int, effectively doubling its range which would prevent an overflow due to an addition, and UINT16_MAX to unsigned int

#include <stdint.h>

uint16_t frame_size_safe;
if ((unsigned)vrc + sizeof(sampling_rate) <= (unsigned)UINT16_MAX - 2) {
    frame_size_safe = vrc + sizeof(sampling_rate);
} else {
    frame_size_safe =  UINT16_MAX - 2;
}

toxav/toxav.c, line 823 at r3 (raw file):

        while ((pkt = vpx_codec_get_cx_data(call->video.second->encoder, &iter))) {
            if (pkt->kind == VPX_CODEC_CX_FRAME_PKT) {
                uint32_t frame_size_safe = pkt->data.frame.sz;

Let's do the check here too.

  • pkt->data.frame.sz is of type size_t, so it's not a fixed type and it can vary from platform to platform.
  • It's unsigned

There might occur an overflow if size_t is greater than 32-bits, so a safer way would be to do

uint16_t frame_size_safe;
if (pkt->data.frame.sz <= UINT16_MAX - 2) {
    frame_size_safe = data.frame.sz;
} else {
    frame_size_safe = UINT16_MAX - 2;
}

Comments from Reviewable

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 24, 2017

You could argue that Opus would never return a value bigger than INT32_MAX or that pkt->data.frame.sz is always at most UINT32_MAX, but I like to stay on the safe side and avoid any potential overflows, and going purely off type ranges those could occur, no matter how unlikely it is.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 25, 2017

Huh, the network test keeps failing and failing on Travis, this is like the fourth time Travis runs. I wonder if it has something to do with the recent network code changes, this failure seems to be way too consistent than your regular random test failures on Travis. Funny enough, it passes on FreeBSD qemu.

LOGGER_WARNING(av->m->log, "Could not send video frame: %s\n", strerror(errno));
rc = TOXAV_ERR_SEND_FRAME_RTP_FAILED;
goto END;
if (rtp_send_data(call->video.first, (const uint8_t *)pkt->data.frame.buf, pkt->data.frame.sz, av->m->log) < 0) {

This comment has been minimized.

@zoff99

zoff99 Nov 25, 2017

you are not using frame_size_safe here. so you disabled the fix again

This comment has been minimized.

@nurupo

nurupo Nov 25, 2017

Member

Good catch, fixed.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 25, 2017

I wonder if mannol wants to do some other patch for this. Splitting a frame into pieces sounds better than just truncating the frame and throwing away the rest.

@zoff99

This comment has been minimized.

zoff99 commented Nov 26, 2017

just for documentation, this is not the PR i wrote. it has been rewritten.

@nurupo

This comment has been minimized.

Member

nurupo commented Nov 26, 2017

That's right, I have rewritten git history to remove lots of merge commits, removing potential integer overflows, addeding some comments and writing a commit message on the way. You can still find the original code in Reviewable, it's pretty similar and it wouldn't happen without zoff finding the issue and making the PR fixing it, so it only makes sense to keep zoff's commit authorship.

Send maximum possible frame data if it's going to overflow
There is a bug in RTP that limits the frame size to UINT16_MAX bytes, but a
frame migh in fact exceed this size limit, and when coverting the actual size
to uint16_t an overflow occurs, resulting in us sending a small portion of the
frame or nothing at all, if the size overflows to be 0. A proper fix would
likely break the toxav protocol, but we can improve the current behaviour
without breaking the protocol. We detect when the overflow occurs and instead
of letting it happen we send as much of the frame data as we can, avoiding the
cases when we would send less or none at all due to the overflow.
@nurupo

This comment has been minimized.

Member

nurupo commented Nov 26, 2017

Reviewed 1 of 1 files at r4, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


toxav/toxav.c, line 721 at r3 (raw file):

Previously, nurupo wrote…

Let's do bound checking to make sure no overflow can occur here.

  • vrc is of type int, meaning it's at least 16 bits.
  • vrc can't be negative, we checked this above.
  • vrc is signed, so it has only half of its positive range.
  • sizeof(sampling_rate) is 4.

There might be an issue if int is 64-bit or greater. A signed 64-bit integer can go as far as (2^63)-1, while uint32_t can hold at most (2^32)-1, meaning that frame_size_safe might overflow due to possible type narrowing.

The safe version of this code would be to do

#include <stdint.h>

// make sure vrc + sizeof(sampling_rate) <= UINT16_MAX - 2
uint16_t frame_size_safe;
if (vrc <= UINT16_MAX - 2 - sizeof(sampling_rate)) {
    frame_size_safe = vrc;
} else {
    frame_size_safe = UINT16_MAX - 2;
}

or with casts, promoting non-negative int vrc to unsigned int, effectively doubling its range which would prevent an overflow due to an addition, and UINT16_MAX to unsigned int

#include <stdint.h>

uint16_t frame_size_safe;
if ((unsigned)vrc + sizeof(sampling_rate) <= (unsigned)UINT16_MAX - 2) {
    frame_size_safe = vrc + sizeof(sampling_rate);
} else {
    frame_size_safe =  UINT16_MAX - 2;
}

That first code fragment should have said frame_size_safe = vrc + sizeof(sampling_rate); instead of just frame_size_safe = vrc;


Comments from Reviewable

@robinlinden

This comment has been minimized.

Member

robinlinden commented Dec 17, 2017

Closing this as #623 fixes the same issue by sending the payload in several packets instead of truncating it.

@SkyzohKey SkyzohKey moved this from In Progress to Done in Video Feb 13, 2018

@iphydf iphydf added this to the v0.1.11 milestone Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment