WiseLibs / better-sqlite3

The fastest and simplest library for SQLite3 in Node.js.
MIT License
5.47k stars 396 forks source link

db::serialize() crashing with Electron #981

Open terrakuh opened 1 year ago

terrakuh commented 1 year ago

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 commented 1 year ago

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 https://github.com/WiseLibs/better-sqlite3/issues/858#issuecomment-1206184217

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 commented 1 year ago

image

https://github.com/WiseLibs/better-sqlite3/blob/37b7714ace3792a9122ec7e67ceda5a28b1d00a3/src/objects/database.lzz#L285-L294

@JoshuaWise do you agree with ChatGPT?

Prinzhorn commented 1 year ago

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 commented 1 year ago

Also see https://github.com/electron/electron/issues/35801 (https://github.com/electron/electron/issues/35801#issuecomment-1264466677), because you're probably thinking "Great, now we need twice the memory in Node.js as well just to fix this for Electron"

terrakuh commented 1 year ago

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

terrakuh commented 1 year ago

Sorry, I forgot to mention my versions:

terrakuh commented 1 year ago

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 commented 1 year ago

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 commented 3 months ago

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?