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

napi: zero length typedarray in 12 vs 13 #32089

Closed
mafintosh opened this issue Mar 4, 2020 · 3 comments
Closed

napi: zero length typedarray in 12 vs 13 #32089

mafintosh opened this issue Mar 4, 2020 · 3 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@mafintosh
Copy link
Member

We just hit this bug, which may be a feature, but at least not documented where in Node.js
13, the napi_get_typedarray_info returns NULL as the data pointer for 0 length typedarrays but Node.js 12 returns a pointer.

This C program shows the difference:

#include <node_api.h>
#include <stdio.h>

napi_value print (napi_env env, napi_callback_info info) {
  napi_value argv[1];
  size_t argc = 1;

  napi_get_cb_info(env, info, &argc, argv, NULL, NULL);

  // https://nodejs.org/dist/latest-v13.x/docs/api/n-api.html#n_api_napi_get_typedarray_info

  napi_typedarray_type type;
  napi_value array_buffer;
  size_t len;
  void *data;
  size_t offset;
  napi_get_typedarray_info(env, argv[0], &type, &len, &data, &array_buffer, &offset);

  printf("buffer is %zu\n", data);

  return NULL;
}

napi_value init_all (napi_env env, napi_value exports) {
  napi_value print_fn;
  napi_create_function(env, NULL, 0, print, NULL, &print_fn);
  napi_set_named_property(env, exports, "print", print_fn);
  return exports;
}

NAPI_MODULE(NODE_GYP_MODULE_NAME, init_all)

Compile it and run it with

require('./binding...').print(Buffer.alloc(0))

Node.js 12 returns a pointer and 13 NULL.

@richardlau richardlau added the node-api Issues and PRs related to the Node-API. label Mar 4, 2020
@addaleax
Copy link
Member

addaleax commented Mar 4, 2020

… what is the (un)expected behaviour here? It seems perfectly fine to me to return any pointer for a zero-length ArrayBuffer, including NULL.

(I’m guessing that this is happening because of V8’s BackingStore migration, but in this case that should just be an implementation detail.)

@legendecas
Copy link
Member

It seems perfectly fine to me to return any pointer for a zero-length ArrayBuffer, including NULL.

In the case, the length shall always be respected even if the pointer is not NULL. But that is an elementary check over the return values, documenting it might increase the verbosity of the document.

@mafintosh
Copy link
Member Author

We had logic there we checked if a pointer was NULL to see we had load it from the typed_array method, which this broke. As mentioned, it might be a feature but having it documented would have saved us debugging.

mhdawson added a commit to mhdawson/io.js that referenced this issue Apr 1, 2020
Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

Fixes: nodejs#32089
targos pushed a commit that referenced this issue Apr 12, 2020
Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

Fixes: #32089

PR-URL: #32603
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 14, 2020
Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

Fixes: #32089

PR-URL: #32603
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Apr 22, 2020
Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

Fixes: #32089

PR-URL: #32603
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants