fix: add maximum length limit to prevent OOM (resolves #4409)#4731
fix: add maximum length limit to prevent OOM (resolves #4409)#4731souravk29 wants to merge 7 commits intoboa-dev:mainfrom
Conversation
|
i am working to solve the broken tests, please leave your suggestions if any |
Test262 conformance changes
Broken tests (23): |
|
@jedel1043 please review the PR and help me pass these failing tests .i have been trying to resolve it but not getting to the endpoint, i tried by modifying the is_object() to is_null_or_undefined() in mod.rs names-valid.js (previously Ignored)
Broken tests (10):
test/intl402/Intl/getCanonicalLocales/unicode-ext-canonicalize-yes-to-true.js (previously Passed)
test/intl402/PluralRules/prototype/select/notation.js (previously Passed)
test/built-ins/RegExp/prototype/exec/regexp-builtin-exec-v-u-flag.js (previously Passed)
test/built-ins/String/prototype/replace/regexp-prototype-replace-v-u-flag.js (previously Passed)
test/built-ins/String/prototype/matchAll/regexp-prototype-matchAll-v-u-flag.js (previously Passed)
test/built-ins/String/prototype/search/regexp-prototype-search-v-u-flag.js (previously Passed)
test/built-ins/String/prototype/search/regexp-prototype-search-v-flag.js (previously Passed)
test/built-ins/String/prototype/match/regexp-prototype-match-v-u-flag.js (previously Passed)
test/staging/sm/RegExp/unicode-raw.js (previously Passed)
test/staging/sm/RegExp/unicode-class-raw.js (previously Passed) . |
|
my latest commit causes 23 failures(prev 10). please guide |
There was a problem hiding this comment.
In general i believe you changed a few places too many which also caused your test failures with the primitives (see comments below) .
At the same time you missed some places including but not limited to core/string/src/builder.rs (which in the current version will still successfully build a JsString that is too long). Additionally, the replace function still does separate length checking which i think should be get rid of with a PR like this (although that require another way of implementing the function ig).
| // | ||
| // `allocate_seq` must return a valid pointer to newly allocated memory, meaning | ||
| // `ptr` and all `string`s should never overlap. | ||
| // SAFETY: See original safety comments |
There was a problem hiding this comment.
well i think it would be better if you had just kept the original comment?
|
|
||
| // Validate against maximum string length BEFORE attempting allocation | ||
| if count > MAX_STRING_LENGTH { | ||
| alloc_overflow(); |
There was a problem hiding this comment.
is there a reason you let it panic here? seems to me that it should be a recoverable error here.
| // 2. If searchValue is neither undefined nor null, then | ||
| if !search_value.is_null_or_undefined() { | ||
| // a. Let replacer be ? GetMethod(searchValue, @@replace). |
There was a problem hiding this comment.
this path is not for primitives so you need to check if it is an object.
i fail to understand why you modified this in the first place. this does not have anything to do with MAX_STRING_LENGTH ?
if you change all this back, it will solve your failing tests problem.
EDIT: it seems that is_object check was added by #4665 , so probably you branched before that and kept the old version of !search_value.is_null_or_undefined() and then got hit by failed tests.
| // 2. If searchValue is neither undefined nor null, then | ||
| if !search_value.is_null_or_undefined() { | ||
| // a. Let isRegExp be ? IsRegExp(searchValue). |
There was a problem hiding this comment.
also check is_object here
| if !regexp.is_null_or_undefined() { | ||
| // a. Let matcher be ? GetMethod(regexp, @@match). |
| if !regexp.is_null_or_undefined() { | ||
| // a. Let isRegExp be ? IsRegExp(regexp). |
| if !regexp.is_null_or_undefined() { | ||
| // a. Let searcher be ? GetMethod(regexp, @@search). |
| Err(_) => alloc_overflow(), // Keep existing panic behavior for now | ||
| } |
There was a problem hiding this comment.
well why keep this for now? the point was to make the construction of JsStrings fallible
| // Numeric addition - KEEP ORIGINAL LOGIC | ||
| match (x.to_numeric(context)?, y.to_numeric(context)?) { |
There was a problem hiding this comment.
this looks like a comment that should have been kept to yourself and not part of a PR.
| pub(crate) fn try_allocate(len: usize) -> Result<NonNull<Self>, Option<Layout>> { | ||
| // Validate length BEFORE attempting Layout calculation | ||
| if len > crate::MAX_STRING_LENGTH { | ||
| return Err(None); // Signal overflow error | ||
| } | ||
| // Calculate layout using the original method |
There was a problem hiding this comment.
hmm this check looks abundant and at the wrong layer. the attempted string length should have been checked already before try_allocate is called.
|
what @zhuzhu81998 said |
|
Also I'm just gonna say that we kinda don't need to return an error on the EDIT: we should also remove all uses of |
3 participants
resolves #4409
for :
reached state:
i tried to solve this issue, please tell me further changes required