Skip to content

Commit 3ddec35

Browse files
fredmnlfredmnl
committed
review
1 parent bc68eb3 commit 3ddec35

3 files changed

Lines changed: 189 additions & 96 deletions

File tree

‎lib/auth/streamingV4/trailingChecksumTransform.js‎

Lines changed: 39 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ class TrailingChecksumTransform extends Transform {
1616
this.log = log;
1717
this.errCb = errCb;
1818
this.chunkSizeBuffer = Buffer.alloc(0);
19-
this.outputBuffer = Buffer.alloc(0);
20-
this.toFlush = Buffer.alloc(0);
19+
this.outputBuffer = [];
20+
this.bytesToDiscard = 0; // when trailing /r/n are present, we discard them but they can be in different chunks
2121
this.bytesToRead = 0; // when a chunk is advertised, the size is put here and we forward all bytes
2222
this.streamClosed = false;
2323
}
@@ -32,64 +32,66 @@ class TrailingChecksumTransform extends Transform {
3232
*/
3333
_transform(chunkInput, encoding, callback) {
3434
let chunk = chunkInput;
35-
for (;;) {
36-
if (chunk.byteLength === 0) {
37-
break;
38-
}
39-
if (this.streamClosed) {
40-
return callback(null, '', encoding);
35+
while (chunk.byteLength > 0 && !this.streamClosed) {
36+
if (this.bytesToDiscard > 0) {
37+
const toDiscard = Math.min(this.bytesToDiscard, chunk.byteLength);
38+
chunk = chunk.subarray(toDiscard);
39+
this.bytesToDiscard -= toDiscard;
40+
continue;
4141
}
42+
// forward up to bytesToRead bytes from the chunk, restart processing on leftover
4243
if (this.bytesToRead > 0) {
43-
if (chunk.byteLength <= this.bytesToRead) {
44-
// chunk is smaller than the advertised size
45-
// forward the whole chunk
46-
this.bytesToRead -= chunk.byteLength;
47-
this.outputBuffer = Buffer.concat([this.outputBuffer, chunk]);
48-
break;
44+
const toRead = Math.min(this.bytesToRead, chunk.byteLength);
45+
this.outputBuffer.push(chunk.subarray(0, toRead));
46+
chunk = chunk.subarray(toRead);
47+
this.bytesToRead -= toRead;
48+
if (this.bytesToRead === 0) {
49+
this.bytesToDiscard = 2;
4950
}
50-
// chunk is bigger than the advertised size
51-
this.log.info('concatenating chunk', { first: this.outputBuffer.toString() });
52-
this.log.info('concatenating chunk', { second: chunk.subarray(0, this.bytesToRead).toString() });
53-
this.outputBuffer = Buffer.concat([this.outputBuffer, chunk.subarray(0, this.bytesToRead)]);
54-
chunk = chunk.subarray(this.bytesToRead);
55-
this.bytesToRead = 0;
5651
continue;
5752
}
58-
const lineBreakIndex = chunk.indexOf('\r\n');
59-
if (lineBreakIndex === -1) {
60-
this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk]);
61-
if (this.chunkSizeBuffer.byteLength > 8) {
53+
54+
const lineBreakIndex2 = chunk.indexOf('\r');
55+
if (lineBreakIndex2 === -1) {
56+
if (this.chunkSizeBuffer.byteLength + chunk.byteLength > 10) {
57+
this.log.debug('chunk size field too big', {
58+
chunkSizeBuffer: this.chunkSizeBuffer.toString('hex'),
59+
truncatedChunk: chunk.subarray(0, 8).toString('hex'),
60+
});
6261
// if bigger, the chunk would be over 5 GB
6362
// returning early to avoid a DoS by memory exhaustion
6463
return callback(errors.InvalidArgument);
6564
}
66-
return callback(null, '', encoding);
67-
}
68-
if (lineBreakIndex === 0) {
69-
chunk = chunk.subarray(lineBreakIndex + 2);
70-
continue;
65+
// no delimiter, we'll keep the chunk for later
66+
this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk]);
67+
break;
7168
}
72-
this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk.subarray(0, lineBreakIndex)]);
73-
chunk = chunk.subarray(lineBreakIndex + 2);
69+
70+
this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk.subarray(0, lineBreakIndex2)]);
71+
chunk = chunk.subarray(lineBreakIndex2);
72+
7473
// chunk-size is sent in hex
7574
const dataSize = Number.parseInt(this.chunkSizeBuffer.toString(), 16);
7675
if (Number.isNaN(dataSize)) {
76+
this.log.debug('unable to parse chunk size', {
77+
chunkSizeBuffer: this.chunkSizeBuffer.toString('hex'),
78+
});
7779
return callback(errors.InvalidArgument);
7880
}
7981
this.chunkSizeBuffer = Buffer.alloc(0);
8082
if (dataSize === 0) {
83+
// TODO: check if the checksum is correct
8184
// last chunk, no more data to read, the stream is closed
8285
this.streamClosed = true;
83-
break;
8486
}
8587
this.bytesToRead = dataSize;
86-
this.log.info('chunk size', { dataSize });
88+
this.bytesToDiscard = 2;
8789
}
8890

89-
this.toFlush = this.outputBuffer;
90-
this.outputBuffer = Buffer.alloc(0);
91-
this.log.info('toFlush', { toFlush: this.toFlush.toString() });
92-
return callback(null, this.toFlush, encoding);
91+
// TODO: push the stream into a checksum accumulator
92+
const toFlush = Buffer.concat(this.outputBuffer);
93+
this.outputBuffer = [];
94+
return callback(null, toFlush, encoding);
9395
}
9496
}
9597

‎tests/functional/raw-node/test/trailingChecksums.js‎

Lines changed: 41 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ const bucket = 'testunsupportedchecksumsbucket';
77
const objectKey = 'key';
88
const objData = Buffer.alloc(1024, 'a');
99
// note this is not the correct checksum in objDataWithTrailingChecksum
10-
const objDataWithTrailingChecksum = '10\r\n0123456789abcdef\r\n0\r\nx-amz-checksum-crc64nvme:YeIDuLa7tU0=\r\n';
11-
const objDataWithoutTrailingChecksum = '0123456789abcdef';
10+
const objDataWithTrailingChecksum = '10\r\n0123456789abcdef\r\n10\r\n0123456789abcdef\r\n0\r\nx-amz-checksum-crc64nvme:YeIDuLa7tU0=\r\n';
11+
const objDataWithoutTrailingChecksum = '0123456789abcdef0123456789abcdef';
1212

1313
const authCredentials = {
1414
accessKey: 'accessKey1',
@@ -48,69 +48,51 @@ describe('trailing checksum requests:', () => {
4848
});
4949
});
5050

51-
const objDataChunkings = [
52-
[objDataWithTrailingChecksum],
53-
[objDataWithTrailingChecksum.substring(0, 1), objDataWithTrailingChecksum.substring(1)],
54-
[objDataWithTrailingChecksum.substring(0, 2), objDataWithTrailingChecksum.substring(2)],
55-
[objDataWithTrailingChecksum.substring(0, 3), objDataWithTrailingChecksum.substring(3)],
56-
[objDataWithTrailingChecksum.substring(0, 4), objDataWithTrailingChecksum.substring(4)],
57-
[objDataWithTrailingChecksum.substring(0, 10), objDataWithTrailingChecksum.substring(10)],
58-
[objDataWithTrailingChecksum.substring(0, 20), objDataWithTrailingChecksum.substring(20)],
59-
[objDataWithTrailingChecksum.substring(0, 21), objDataWithTrailingChecksum.substring(21)],
60-
[objDataWithTrailingChecksum.substring(0, 22), objDataWithTrailingChecksum.substring(22)],
61-
];
62-
63-
objDataChunkings.forEach((objDataChunks, index) => {
64-
it(`should accept unsigned trailing checksum, chunk configuration ${index + 1}`, done => {
65-
const req = new HttpRequestAuthV4(
66-
`http://localhost:8000/${bucket}/${objectKey}`,
67-
Object.assign(
68-
{
69-
method: 'PUT',
70-
headers: {
71-
'content-length': objDataWithTrailingChecksum.length,
72-
'x-amz-decoded-content-length': objDataWithoutTrailingChecksum.length,
73-
'x-amz-content-sha256': 'STREAMING-UNSIGNED-PAYLOAD-TRAILER',
74-
'x-amz-trailer': 'x-amz-checksum-crc64nvme',
75-
},
51+
it('should accept unsigned trailing checksum', done => {
52+
const req = new HttpRequestAuthV4(
53+
`http://localhost:8000/${bucket}/${objectKey}`,
54+
Object.assign(
55+
{
56+
method: 'PUT',
57+
headers: {
58+
'content-length': objDataWithTrailingChecksum.length,
59+
'x-amz-decoded-content-length': objDataWithoutTrailingChecksum.length,
60+
'x-amz-content-sha256': 'STREAMING-UNSIGNED-PAYLOAD-TRAILER',
61+
'x-amz-trailer': 'x-amz-checksum-crc64nvme',
7662
},
77-
authCredentials
78-
),
79-
res => {
80-
assert.strictEqual(res.statusCode, 200);
81-
res.on('data', () => {});
82-
res.on('end', done);
83-
}
84-
);
63+
},
64+
authCredentials
65+
),
66+
res => {
67+
assert.strictEqual(res.statusCode, 200);
68+
res.on('data', () => {});
69+
res.on('end', done);
70+
}
71+
);
8572

86-
req.on('error', err => {
87-
assert.ifError(err);
88-
});
73+
req.on('error', err => {
74+
assert.ifError(err);
75+
});
8976

90-
// write the data in two chunks
91-
objDataChunks.forEach(chunk => {
92-
req.write(chunk);
93-
}
94-
);
77+
req.write(objDataWithTrailingChecksum);
9578

96-
req.once('drain', () => {
97-
req.end();
98-
});
79+
req.once('drain', () => {
80+
req.end();
9981
});
82+
});
10083

101-
it('should have correct object content for unsigned trailing checksum', done => {
102-
makeS3Request({
103-
method: 'GET',
104-
authCredentials,
105-
bucket,
106-
objectKey,
107-
}, (err, res) => {
108-
assert.ifError(err);
109-
assert.strictEqual(res.statusCode, 200);
110-
// check that the object data is the input stripped of the trailing checksum
111-
assert.strictEqual(res.body, objDataWithoutTrailingChecksum);
112-
return done();
113-
});
84+
it('should have correct object content for unsigned trailing checksum', done => {
85+
makeS3Request({
86+
method: 'GET',
87+
authCredentials,
88+
bucket,
89+
objectKey,
90+
}, (err, res) => {
91+
assert.ifError(err);
92+
assert.strictEqual(res.statusCode, 200);
93+
// check that the object data is the input stripped of the trailing checksum
94+
assert.strictEqual(res.body, objDataWithoutTrailingChecksum);
95+
return done();
11496
});
11597
});
11698

‎tests/unit/auth/TrailingChecksumTransform.js‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
const assert = require('assert');
2+
const async = require('async');
3+
const { Readable } = require('stream');
4+
5+
const TrailingChecksumTransform = require('../../../lib/auth/streamingV4/trailingChecksumTransform');
6+
const { DummyRequestLogger } = require('../helpers');
7+
8+
const log = new DummyRequestLogger();
9+
10+
// note this is not the correct checksum in objDataWithTrailingChecksum
11+
const objDataWithTrailingChecksum = '10\r\n0123456789abcdef\r\n' +
12+
'2\r\n01\r\n' +
13+
'1\r\n2\r\n' +
14+
'd\r\n3456789abcdef\r\n' +
15+
'0\r\nchecksum:xyz=\r\n';
16+
const objDataWithoutTrailingChecksum = '0123456789abcdef0123456789abcdef';
17+
18+
class ChunkedReader extends Readable {
19+
constructor(chunks) {
20+
super();
21+
this._parts = chunks;
22+
this._index = 0;
23+
}
24+
25+
_read() {
26+
if (this._index >= this._parts.length) {
27+
this.push(null);
28+
return;
29+
}
30+
this.push(this._parts[this._index]);
31+
this._index++;
32+
}
33+
}
34+
35+
describe('TrailingChecksumTransform class', () => {
36+
it(`should correctly remove checksums`, done => {
37+
const trailingChecksumTransform = new TrailingChecksumTransform(log, err => {
38+
assert.strictEqual(err, null);
39+
});
40+
const chunks = [
41+
Buffer.from(objDataWithTrailingChecksum),
42+
];
43+
const chunkedReader = new ChunkedReader(chunks);
44+
chunkedReader.pipe(trailingChecksumTransform);
45+
const outputChunks = [];
46+
trailingChecksumTransform.on('data', (chunk) => outputChunks.push(Buffer.from(chunk)));
47+
trailingChecksumTransform.on('finish', () => {
48+
const data = Buffer.concat(outputChunks).toString();
49+
assert.strictEqual(data, objDataWithoutTrailingChecksum);
50+
done();
51+
});
52+
trailingChecksumTransform.on('error', err => {
53+
assert.ifError(err);
54+
});
55+
});
56+
57+
// test all bisection of the input string
58+
async.forEach([...Array(objDataWithTrailingChecksum.length).keys()], (i) => {
59+
it(`should correctly remove checksums, cut at ${i}`, done => {
60+
const trailingChecksumTransform = new TrailingChecksumTransform(log, err => {
61+
assert.strictEqual(err, null);
62+
});
63+
const chunks = [
64+
Buffer.from(objDataWithTrailingChecksum.substring(0, i)),
65+
Buffer.from(objDataWithTrailingChecksum.substring(i)),
66+
];
67+
const chunkedReader = new ChunkedReader(chunks);
68+
chunkedReader.pipe(trailingChecksumTransform);
69+
const outputChunks = [];
70+
trailingChecksumTransform.on('data', (chunk) => outputChunks.push(Buffer.from(chunk)));
71+
trailingChecksumTransform.on('finish', () => {
72+
const data = Buffer.concat(outputChunks).toString();
73+
assert.strictEqual(data, objDataWithoutTrailingChecksum);
74+
done();
75+
});
76+
trailingChecksumTransform.on('error', err => {
77+
assert.ifError(err);
78+
});
79+
});
80+
});
81+
82+
// test all trisection of the input string
83+
async.forEach([...Array(objDataWithTrailingChecksum.length - 2).keys()], (i) => {
84+
async.forEach([...Array(objDataWithTrailingChecksum.length - i - 2).keys()], (j) => {
85+
it(`should correctly remove checksums, cut at ${i} and ${i + j + 1}`, done => {
86+
const trailingChecksumTransform = new TrailingChecksumTransform(log, err => {
87+
assert.strictEqual(err, null);
88+
});
89+
const chunks = [
90+
Buffer.from(objDataWithTrailingChecksum.substring(0, i + 1)),
91+
Buffer.from(objDataWithTrailingChecksum.substring(i + 1, i + j + 2)),
92+
Buffer.from(objDataWithTrailingChecksum.substring(i + j + 2)),
93+
];
94+
const chunkedReader = new ChunkedReader(chunks);
95+
chunkedReader.pipe(trailingChecksumTransform);
96+
const outputChunks = [];
97+
trailingChecksumTransform.on('data', (chunk) => outputChunks.push(Buffer.from(chunk)));
98+
trailingChecksumTransform.on('finish', () => {
99+
const data = Buffer.concat(outputChunks).toString();
100+
assert.strictEqual(data, objDataWithoutTrailingChecksum);
101+
done();
102+
});
103+
trailingChecksumTransform.on('error', err => {
104+
assert.ifError(err);
105+
});
106+
});
107+
});
108+
});
109+
});

0 commit comments

Comments
 (0)