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.

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

[RangeError] use missing size parameter #11

Closed
goliatone opened this issue Oct 17, 2022 · 6 comments
Closed

[RangeError] use missing size parameter #11

goliatone opened this issue Oct 17, 2022 · 6 comments

Comments

@goliatone
Copy link

Using this serde declaration:

sd.use(sd.array(sd.bytes(sd.uint32), sd.uint32));

I get the following error:

RangeError: offset is out of bounds
    at Uint8Array.set (<anonymous>)
    at Object.ser (.../node_modules/sirdez/src/serdes/bytes.ts:9:17)
    at .../node_modules/sirdez/src/serdes/array.ts:10:12
    at contextSer (.../node_modules/sirdez/src/context.ts:27:7)
    at Object.toBytes (.../node_modules/sirdez/src/use.ts:14:34)
    at serialize (.../src/datasources/srs.ts:391:42)
    at CacheClient.set (.../src/lib/redis-cache/cache.js:203:48)
    at CacheClient.tryGet (.../src/lib/redis-cache/cache.js:131:24)

Looking at the source code I should be able to create a context with a larger size than the default value. For that I would expect to call use second parameter, size with a size.

However, the signature available for sd.use in my current project is this:

function use<T>(
  { ser, des }: Serdes<T>
): UsableSerdes<T>

I was expecting to have something like this:

function use<T>(
  { ser, des }: Serdes<T>,
  size?: number
): UsableSerdes<T>
  • package version: 0.0.6

What am I doing wrong?

@weisrc
Copy link
Owner

weisrc commented Oct 17, 2022

Yes, the package is outdated, I haven't published to NPM yet. I need to make sure everything is right before the push. Although, it is strange that you are getting this error. The array buffer should grow by itself upon encountering this error. I will investigate this error tomorrow if I have time and publish it.

@azerum
Copy link
Contributor

azerum commented Oct 28, 2022

I was able to reproduce this bug with the following code:

import * as sd from 'sirdez';

const contextByteLength = 4096;
const headByteLength = 2;

const data = new Uint8Array(contextByteLength - headByteLength + 1);
const serdes = sd.use(sd.bytes(sd.uint16));

serdes.toBytes(data);

This code throws exception RangeError: offset is out of bounds. The exception happens here because data array is too big to fit into context.

The context size is the default one used in code - 4096 bytes, 2 bytes are taken for head, so we have only 4094 bytes left. The data array is one byte bigger, so it doesn't fit, and Uint8Array.set(data, offset) throws error with misleading message offset is out of bounds. Actually, the offset is in bounds, it just that data doesn't fit the buffer.

Next, the exception is caught here. When serializing data, there might happen 2 types of errors: error caused by small size of the buffer, and some user error unrelated to the buffer. If error is caused by the buffer size, we need to ignore it and grow the buffer. If error is a user error, we need to re-throw it.

The code assumes that if ctx.i (i.e. the last offset) is smaller than the limit of the current buffer, then error is the user error. In this case this is not true, because, as I've written earlier, the last offset is actually in buffer bounds: the error happens because the data doesn't fit fully into buffer.

So, maybe we should replace if (ctx.i < limit) throw error; on that line with

if (!(error instanceof RangeError)) throw error;

That is, assume that every RangeError is caused by the buffer size and ignore it?

@weisrc
Copy link
Owner

weisrc commented Oct 28, 2022

Another solution that will make use of the if guard if (ctx.i < limit) throw error is to increment the pointer i before Uint8Array.set(data, offset).

I think adding both the limit check and the instanceof check will further narrow the errors that are actually caused by out-of-bounds error.

@weisrc
Copy link
Owner

weisrc commented Oct 28, 2022

I hope this issue only concerns bytes.ser.

weisrc added a commit that referenced this issue Oct 28, 2022
@weisrc
Copy link
Owner

weisrc commented Oct 28, 2022

I made some changes and some tests reflecting the suggestion above, hopefully, it is fine. #11 (comment)

@weisrc weisrc closed this as completed Oct 28, 2022
github-actions bot added a commit that referenced this issue Oct 28, 2022
@azerum
Copy link
Contributor

azerum commented Oct 28, 2022

It seems like in Uint8Array, only set() method can throw error if input data is too large, and my code search says that set() is used only in bytes.ser, so the solution should be fine. Though it's easy to forget about this trick if someday some other code will use set() again.

Maybe add warning for Uint8Array.set() with this ESLint rule? Something like "Context pointer must be incremented before calling Uint8Array.set(), see issue #11".

Or set rule level to error and suggest using a helper function instead. The helper function would take Context as parameter, increment the pointer, and then call `set()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants