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.

Sign up for GitHub

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

Use lengthCode to determine whether to read sequenceLength again #101

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Use lengthCode to determine whether to read sequenceLength again #101

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c48c4f0
only use emHeader size when lengthCode
snosenzo Nov 14, 2023
44ddd9e
remove comment and add comment for useEmHeaderlenght
snosenzo Nov 15, 2023
4eb37a3
update cdr dep and add tests
snosenzo Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions packages/omgidl-serialization/src/MessageReader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,8 @@ module builtin_interfaces {
writer.emHeader(true, 1, 8); // heightMeters emHeader
writer.float64(data.heightMeters);
writer.emHeader(true, 2, 4 + 4 + 1); // address emHeader
// dHeader for inner object not written again because the object size is already specified in the emHeader
writer.emHeader(true, 1, 1); // pointer emHeader
// dHeader for inner object not written again because the object size is already specified in the emHeader and the lengthCode of 5 allows it to not be written again
writer.emHeader(true, 1, 1, 5); // pointer emHeader
Comment on lines +495 to +496
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the reader supposed to be able to handle this both ways (length code ≥5 or <5)? If so, should we have another test which uses a length code <5?

(same applies to below test as well)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's correct. I'll add some more tests to account for this.

writer.uint8(data.address.pointer);
writer.emHeader(true, 3, 1); // age emHeader
writer.uint8(data.age);
Expand Down Expand Up @@ -593,12 +593,12 @@ module builtin_interfaces {
};

writer.dHeader(1);
writer.emHeader(true, 1, data.name.length + 1); // "name" field emHeader. add 1 for null terminator
writer.string(data.name, false); // don't write length again
writer.emHeader(true, 2, 3 * 8); // xValues emHeader
writer.float64Array(data.xValues, false); // do not write length of array again. Already included in emHeader
writer.emHeader(true, 1, data.name.length + 1, 2); // "name" field emHeader. add 1 for null terminator
writer.string(data.name, true); // need to write length because lengthCode < 5
writer.emHeader(true, 2, 3 * 8, 7); // xValues emHeader
writer.float64Array(data.xValues, false); // do not write length of array again. Already included in emHeader when lengthCode is 7

writer.emHeader(true, 3, 3 * 8); // yValues emHeader
writer.emHeader(true, 3, 3 * 8, 7); // yValues emHeader, lengthCode = 7 means we don't have to write sequenceLength
writer.float64Array(data.yValues, false); // do not write length of array again. Already included in emHeader

writer.emHeader(true, 4, 4); // count emHeader
Expand Down Expand Up @@ -655,8 +655,9 @@ module builtin_interfaces {
numbers: [],
};

writer.emHeader(true, 1, data.numbers.length + 4); // writes 4 because the sequence length is after it
writer.sequenceLength(0);
writer.dHeader(1); // dHeader of struct
writer.emHeader(true, 1, data.numbers.length + 4, 2); // writes 4 because the sequence length is after it
writer.sequenceLength(0); // Because its lengthCode < 5

const rootDef = "Array";
const reader = new MessageReader(rootDef, parseIDL(msgDef));
Expand Down
24 changes: 10 additions & 14 deletions packages/omgidl-serialization/src/MessageReader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,11 @@ export class MessageReader<T = unknown> {
): unknown {
const { readMemberHeader, parentName } = emHeaderOptions;
const definitionId = getDefinitionId(field);
let emHeaderSizeBytes = undefined;
let emHeaderSizeBytes;
if (readMemberHeader) {
const { id, objectSize: objectSizeBytes } = reader.emHeader();
emHeaderSizeBytes = objectSizeBytes;
const { id, objectSize: objectSizeBytes, lengthCode } = reader.emHeader();
emHeaderSizeBytes = useEmHeaderAsLength(lengthCode) ? objectSizeBytes : undefined;

// this can help spot misalignments in reading the data
if (definitionId != undefined && id !== definitionId) {
throw Error(
Expand Down Expand Up @@ -236,8 +237,6 @@ export class MessageReader<T = unknown> {
}
const headerSpecifiedLength =
emHeaderSizeBytes != undefined ? Math.floor(emHeaderSizeBytes / typeLength) : undefined;
const remainderBytes =
emHeaderSizeBytes != undefined ? emHeaderSizeBytes % typeLength : undefined;

if (field.isArray === true) {
const deser = typedArrayDeserializers.get(field.type);
Expand All @@ -261,15 +260,7 @@ export class MessageReader<T = unknown> {
// last arrayLengths length is handled in deserializer. It returns an array
return readNestedArray(typedArrayDeserializer, arrayLengths.slice(0, -1), 0);
} else {
// Special case can happen where the emHeader is not divisible by the typeLength and there is a remainder of 4 bytes.
// 4 bytes of 0's are printed after the header length
// This can happen for 8 byte typeLength types.
Comment on lines -264 to -266
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this just wrong? or where is the sequence length getting read now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was just wrong because I didn't know about the lengthCode caveat. Previously I was always using emHeader size instead of sequenceLength. This case would've arisen for when the lengthCode was <5 and the object size was less than the size of the type. So there was an emHeader object size of 4 and a sequence length. Now that I'm accounting for length code, I will read the sequenceLength earlier in the array handling because the emHeaderSize isn't being used.

if (arrayLengths[0] === 0 && remainderBytes === 4) {
reader.sequenceLength();
return deser(reader, 0);
} else {
return deser(reader, arrayLengths[0]!);
}
return deser(reader, arrayLengths[0]!);
}
} else {
const deser = deserializers.get(field.type);
Expand Down Expand Up @@ -422,3 +413,8 @@ function getCaseForDiscriminator(
}
return unionDef.defaultCase;
}

/** Only length Codes >= 5 should allow for emHeader sizes to be used in place of other integer lengths */
function useEmHeaderAsLength(lengthCode: number | undefined): boolean {
return lengthCode != undefined && lengthCode >= 5;
}