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

db::serialize() crashing with Electron #981

Open
terrakuh opened this issue Mar 31, 2023 · 9 comments
Open

db::serialize() crashing with Electron #981

terrakuh opened this issue Mar 31, 2023 · 9 comments

Comments

@terrakuh
Copy link

I am trying to use the library with Electron. Whenever I try to call db.serialize() the application crashes with the following error message:

[101656:0331/215718.269583:ERROR:node_bindings.cc(158)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers, or disable the sandbox.

This problem persist with the sandbox option enabled or disabled in Electron, e.g.:

const window = new BrowserWindow({
    width: 800,
    height: 600,
    webPreferences: {
      nodeIntegration: false,
      contextIsolation: true,
      sandbox: false, // or true
      preload: path.join(__dirname, "preload.cjs"),
      }
    })

I can use the database normally (insert or read values), but it crashes when using serialize().

@Prinzhorn
Copy link
Contributor

I cannot reproduce this with better-sqlite3 8.2.0 and Electron 23.2.1 with the following main.js:

const Database = require('better-sqlite3');
const db = new Database(':memory:');

console.log(db.serialize());

What is my repro missing? Does it work with Electron <= 20 for you? If so, then this is possibly caused by the memory cage, which I was hoping better-sqlite3 was not affected by:

https://www.electronjs.org/blog/v8-memory-cage
#858 (comment)

So while Blobs are already using the correct APIs and copy the buffer (instead of passing SQLite's memory directly to Node.js), serialize might not? It does use node::Buffer::New and the tests are passing with Electron, so please provide steps to reproduce.

This problem persist with the sandbox option enabled or disabled in Electron, e.g.:

This is an entirely different sandbox concept. The error message comes from V8. The option you're setting affects renderer sandboxing. But you're using better-sqlite3 in the main script.

@Prinzhorn
Copy link
Contributor

image

unsigned char* data = sqlite3_serialize(db->db_handle, *attached_name, &length, 0);
if (!data && length) {
ThrowError("Out of memory");
return;
}
info.GetReturnValue().Set(
node::Buffer::New(isolate, reinterpret_cast<char*>(data), length, FreeSerialization, NULL).ToLocalChecked()
);

@JoshuaWise do you agree with ChatGPT?

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Apr 1, 2023

Never mind, my repro above created a Buffer of size 0, this repros:

const Database = require('better-sqlite3');
const db = new Database(':memory:');

db.exec('CREATE TABLE foo (id)');

console.log(db.serialize());
[10609:0401/091204.985535:ERROR:node_bindings.cc(158)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers, or disable the sandbox.
/src/issues/electron-serialize/node_modules/electron/dist/electron exited with signal SIGSEGV

@Prinzhorn
Copy link
Contributor

Prinzhorn commented Apr 1, 2023

Also see electron/electron#35801 (electron/electron#35801 (comment)), because you're probably thinking "Great, now we need twice the memory in Node.js as well just to fix this for Electron"

@terrakuh
Copy link
Author

terrakuh commented Apr 1, 2023

I have implemented something similar for node-sqlite3 here. Maybe this could be of inspiration?

@terrakuh
Copy link
Author

terrakuh commented Apr 1, 2023

Sorry, I forgot to mention my versions:

  • Electron 22.1.0
  • better-sqlite3 8.2.0
  • OS Fedora Linux 37

@terrakuh
Copy link
Author

terrakuh commented Apr 1, 2023

Maybe another solution would be to use Buffer<char>::NewOrCopy() as described here and here. I was thinking of something like this (untested):

unsigned char* data = sqlite3_serialize(db->db_handle, *attached_name, &length, SQLITE_SERIALIZE_NOCOPY);

if (data != nullptr) {
  info.GetReturnValue().Set(
    node::Buffer::Copy(isolate, reinterpret_cast<char*>(data), length).ToLocalChecked() 
  );
} else {
  // Need to copy from SQLite3.
  data = sqlite3_serialize(db->db_handle, *attached_name, &length, 0);

  if (data == nullptr) {
    ThrowError("Out of memory");
    return;
  }

  std::unique_ptr<unsigned char, decltype(&sqlite3_free)> data_freer(data, &sqlite3_free);
  auto buffer = node::Buffer::NewOrCopy(isolate, reinterpret_cast<char*>(data), length, &FreeSerialization);

  // Ownership was transferred. Node is now responsible to free this memory.
  if (buffer.data() == reinterpret_cast<char*>(data)) {
    data_freer.release();
  }

  info.GetReturnValue().Set(std::move(buffer).ToLocalChecked());
}

@JoshuaWise
Copy link
Member

As this is an issue with Electron and not Node.js, I won't consider this a bug in better-sqlite3, but I will happily accept a PR that fixes this for Electron users. The PR should utilize #ifdef NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED or #ifdef V8_ENABLE_SANDBOX, so the db.serialize() function isn't made less efficient for regular Node.js users

@TheOneTheOnlyJJ
Copy link

This is happening ti me just now. I'm trying to run this sample code:

import Database from "better-sqlite3";
import { join } from "path";

export const initDatabase = (dbDir: string): void => {
  const dbPath = join(dbDir, "BlackBoxDB");
  const db = new Database(dbPath);
  const dbBuffer = db.serialize();
  console.log(dbBuffer.toString());
  db.close();
};

With:
better-sqlite3 version 11.1.2
electron version 31.0.2
node version 20.15.0

Does anyone have/know a fix for this?

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

No branches or pull requests

4 participants