Skip to content

Commit

Permalink
Verify the response string length overflow and throw a specific error (
Browse files Browse the repository at this point in the history
  • Loading branch information
slvrtrn authored Nov 28, 2024
1 parent 8b41e5b commit 64ea1eb
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import type { ClickHouseClient } from '@clickhouse/client-common'
import { createTestClient } from '@test/utils'

// See https://github.com/ClickHouse/clickhouse-js/issues/106
describe('invalid string length', () => {
let client: ClickHouseClient
afterEach(async () => {
await client.close()
})
beforeEach(async () => {
client = createTestClient()
})

const errorMessageMatcher = jasmine.stringContaining(
'consider limiting the amount of requested rows',
)

// The client will buffer the entire response in a string for non-streamable formats.
// A large amount of system.numbers selected should overflow the max allowed allocation size for a string.
it('should fail with a specific error when the response string is too large - json() call', async () => {
const rs = await client.query({
query: 'SELECT number FROM numbers(50000000)',
format: 'JSON',
})
await expectAsync(rs.json()).toBeRejectedWith(
jasmine.objectContaining({
message: errorMessageMatcher,
}),
)
expect().nothing()
})

it('should fail with a specific error when the response string is too large - text() call', async () => {
const rs = await client.query({
query: 'SELECT number FROM numbers(50000000)',
format: 'JSON',
})
await expectAsync(rs.text()).toBeRejectedWith(
jasmine.objectContaining({
message: errorMessageMatcher,
}),
)
expect().nothing()
})
})
12 changes: 11 additions & 1 deletion packages/client-node/src/utils/stream.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import Stream from 'stream'

// See https://github.com/v8/v8/commit/ea56bf5513d0cbd2a35a9035c5c2996272b8b728
const MaxStringLength = Math.pow(2, 29) - 24

export function isStream(obj: any): obj is Stream.Readable {
return obj !== null && typeof obj.pipe === 'function'
}
Expand All @@ -9,7 +12,14 @@ export async function getAsText(stream: Stream.Readable): Promise<string> {

const textDecoder = new TextDecoder()
for await (const chunk of stream) {
text += textDecoder.decode(chunk, { stream: true })
const decoded = textDecoder.decode(chunk, { stream: true })
if (decoded.length + text.length > MaxStringLength) {
throw new Error(
'The response length exceeds the maximum allowed size of V8 String: ' +
`${MaxStringLength}; consider limiting the amount of requested rows.`,
)
}
text += decoded
}

// flush
Expand Down
12 changes: 11 additions & 1 deletion packages/client-web/src/utils/stream.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// See https://github.com/v8/v8/commit/ea56bf5513d0cbd2a35a9035c5c2996272b8b728
const MaxStringLength = Math.pow(2, 29) - 24

export function isStream(obj: any): obj is ReadableStream {
return (
obj !== null && obj !== undefined && typeof obj.pipeThrough === 'function'
Expand All @@ -13,7 +16,14 @@ export async function getAsText(stream: ReadableStream): Promise<string> {

while (!isDone) {
const { done, value } = await reader.read()
result += textDecoder.decode(value, { stream: true })
const decoded = textDecoder.decode(value, { stream: true })
if (decoded.length + result.length > MaxStringLength) {
throw new Error(
'The response length exceeds the maximum allowed size of V8 String: ' +
`${MaxStringLength}; consider limiting the amount of requested rows.`,
)
}
result += decoded
isDone = done
}

Expand Down

0 comments on commit 64ea1eb

Please sign in to comment.