Skip to content

Conversation

haramj
Copy link
Contributor

@haramj haramj commented Oct 8, 2025

This commit resolves a TODO comment in lib/buffer.js by changing the error thrown by atob() during a potential overflow condition.

Modified lib/buffer.js to instantiate and throw new ERR_INVALID_ARG_VALUE('input', result, 'The string to be decoded is too long.'); for the overflow case (result === -3).

This change makes the atob implementation more robust and developer-friendly.
@anonrig

->

The case for a result of -3 was intended to handle a potential
buffer overflow error. However, the underlying C++ implementation using
simdutf::base64_to_binary does not perform bounds-checking and therefore
can never return an OUTPUT_BUFFER_TOO_SMALL error.

This makes the case -3: block unreachable code. This commit removes
the dead code for correctness and clarity.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Oct 8, 2025
@Renegade334
Copy link
Contributor

simdutf::base64_to_binary can never return OUTPUT_BUFFER_TOO_SMALL, as it doesn't perform bounds-checking of its target. This should just be removed in favour of an assertion in case of an unrecognised error.

@haramj
Copy link
Contributor Author

haramj commented Oct 12, 2025

simdutf::base64_to_binary can never return OUTPUT_BUFFER_TOO_SMALL, as it doesn't perform bounds-checking of its target. This should just be removed in favour of an assertion in case of an unrecognised error.
@Renegade334 I've pushed a new commit. Please let me know if any other changes are needed.

@haramj haramj changed the title buffer: throw RangeError on atob overflow buffer: remove unreachable overflow check in atob Oct 12, 2025
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.57%. Comparing base (b13f24c) to head (f5c78ba).
⚠️ Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60161      +/-   ##
==========================================
+ Coverage   88.53%   88.57%   +0.03%     
==========================================
  Files         703      704       +1     
  Lines      207997   208125     +128     
  Branches    40015    40012       -3     
==========================================
+ Hits       184150   184338     +188     
+ Misses      15864    15815      -49     
+ Partials     7983     7972      -11     
Files with missing lines Coverage Δ
lib/buffer.js 100.00% <100.00%> (ø)

... and 51 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines -1305 to -1307
case -3: // Possible overflow
// TODO(@anonrig): Throw correct error in here.
throw lazyDOMException('The input causes overflow.', 'InvalidCharacterError');
Copy link
Contributor

Choose a reason for hiding this comment

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

An unrecognised error state shouldn't be ignored.

I was thinking something like

    case -3:
      assert.fail('Unrecognized simdutf error');

with internal assert added to the imports in the script header:

@@ -97,10 +97,11 @@ const {
 } = require('internal/util/types');
 const {
   inspect: utilInspect,
 } = require('internal/util/inspect');

+const assert = require('internal/assert');
 const {
   codes: {
     ERR_BUFFER_OUT_OF_BOUNDS,
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_ARG_VALUE,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants