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

suppress traits larger than INT16_MAX bytes #14147

Merged
merged 2 commits into from Oct 8, 2018

Conversation

@birarda
Member

birarda commented Oct 5, 2018

No description provided.

@birarda

This comment has been minimized.

Show comment
Hide comment
@birarda

birarda Oct 5, 2018

Member

Test Plan

See Manuscript ticket 18304 for test plan.

Member

birarda commented Oct 5, 2018

Test Plan

See Manuscript ticket 18304 for test plan.

@birarda birarda added the QA Ready label Oct 5, 2018

@birarda birarda added this to the v0.74.0 milestone Oct 5, 2018

@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@conklin94122

project approved

@hyperlogic

I'm not an expert on the networking code, but rejecting traits that are larger then MAXIMUM_TRAIT_SIZE on the sending side is the right thing to do.

@birarda birarda added the DO NOT MERGE label Oct 5, 2018

@birarda birarda dismissed stale reviews from hyperlogic and conklin94122 via 15b9045 Oct 5, 2018

@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@hifi-gustavo

This comment has been minimized.

Show comment
Hide comment
@ZappoMan

project approved. Note: we already merged this into 73.2 so we might want to fast track this on Dev/QA Approve

@qlilaseguinolaza

This comment has been minimized.

Show comment
Hide comment
@qlilaseguinolaza

qlilaseguinolaza Oct 8, 2018

Currently testing

qlilaseguinolaza commented Oct 8, 2018

Currently testing

@JeffClinton

Adding QA approval green checkmark and tag

@@ -39,6 +39,7 @@ namespace AvatarTraits {
using TraitWireSize = int16_t;
const TraitWireSize DELETED_TRAIT_SIZE = -1;
const TraitWireSize MAXIMUM_TRAIT_SIZE = INT16_MAX;

This comment has been minimized.

@SimonWalton-HiFi

SimonWalton-HiFi Oct 8, 2018

Contributor

Would numeric_limits<TraitWireSize>::max() be cleaner? Also is there a reason for making DELETED_TRAIT_SIZE & MAXIMUM_TRAIT_SIZE themselves of TraitWireSize rather than static const int?

@SimonWalton-HiFi

SimonWalton-HiFi Oct 8, 2018

Contributor

Would numeric_limits<TraitWireSize>::max() be cleaner? Also is there a reason for making DELETED_TRAIT_SIZE & MAXIMUM_TRAIT_SIZE themselves of TraitWireSize rather than static const int?

This comment has been minimized.

@birarda

birarda Oct 8, 2018

Member

There are places where we set a TraitWireSize to these values or compare a TraitWireSize to these values - so it felt like it made more sense to just have them be the same type.

@birarda

birarda Oct 8, 2018

Member

There are places where we set a TraitWireSize to these values or compare a TraitWireSize to these values - so it felt like it made more sense to just have them be the same type.

@@ -337,7 +337,7 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
// grab the last trait versions for this avatar
auto& lastProcessedVersions = _processedTraitVersions[avatarID];
while (traitType != AvatarTraits::NullTrait) {
while (traitType != AvatarTraits::NullTrait && message->getBytesLeftToRead() > 0) {
AvatarTraits::TraitVersion packetTraitVersion;

This comment has been minimized.

@SimonWalton-HiFi

SimonWalton-HiFi Oct 8, 2018

Contributor

I guess > 0 could be >= sizeof (AvatarTraits::TraitVersion)

@SimonWalton-HiFi

SimonWalton-HiFi Oct 8, 2018

Contributor

I guess > 0 could be >= sizeof (AvatarTraits::TraitVersion)

This comment has been minimized.

@birarda

birarda Oct 8, 2018

Member

Good point

@birarda

birarda Oct 8, 2018

Member

Good point

@SimonWalton-HiFi

Looks good. Is it intentional for the two files with whitespace-only changes?

@conklin94122 conklin94122 merged commit dc7a5c0 into highfidelity:master Oct 8, 2018

2 checks passed

default Build finished.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment