Do not return uninitialized box by maRci002 · Pull Request #916 · isar/hive

maRci002 · 2022-02-25T10:03:37Z

#846 fix should be rerolled.

_boxes[name] = newBox;
await newBox.initialize();

_boxes can contain a box which isn't initialized so it is possible that isBoxOpen(name) return true and program can acquire this uninitialized box which can lead serios problems for example box.put called.

Imagine the following situation:

var box = Hive.openBox('box');
var sameBox = Hive.openBox('box');

In this case sameBox can be referenced before it's initialized. However this cannot be achieved so easily because we have to get the reference after _boxes[name] = newBox; and during await newBox.initialize(); call (and also after _manager.open).

There is a period when both isBoxOpen(name) and _openingBoxes.containsKey(name) return true.
Let me a show an example (I won’t be bored with how the dart event loop works, this is the easiest example):

import 'dart:async';
import 'dart:io';

import 'package:hive/hive.dart';
import 'package:hive/src/backend/storage_backend.dart';

import 'package:hive/src/hive_impl.dart';

const boxName = 'test_box';

final completer = Completer();

final f1 = Completer();
final f2 = Completer();

class BackendManagerInterceptor extends BackendManager {
  @override
  Future<StorageBackend> open(
      String name, String? path, bool crashRecovery, HiveCipher? cipher) async {
    final result = await super.open(name, path, crashRecovery, cipher);

    completer.complete();
    return result;
  }
}

void main() async {
  final sep = Platform.pathSeparator;
  final dbDirectory = Directory('${Directory.current.path}${sep}hive_db');

  print('dbDirectory: ${dbDirectory.path}');
  await resetAndInitDatabaseLocationPath(dbDirectory);

  final hive = HiveImpl.debug(BackendManagerInterceptor());
  hive.init(dbDirectory.path);

  var box = hive.openBox(boxName);
  box.then(
    (value) {
      print('Successful init');
      f1.complete();
    },
  );

  completer.future.then((value) {
    var sameBox = hive.openBox(boxName);
    sameBox.then(
      (value) {
        print('Box is avaible before it\'s initialized');
        f2.complete();
      },
    );
  });

  // make sure program doesn't exit before all print executed
  await (Future.wait([f1.future, f2.future]));
}

Future<void> resetAndInitDatabaseLocationPath(Directory dbDirectory) async {
  if (await dbDirectory.exists()) {
    await dbDirectory.delete(recursive: true);
  }
  await dbDirectory.create(recursive: true);
}

There is another fix just place _openingBoxes.containsKey(name) check before _boxes[name] = newBox; however I think there shouldn't be any point in time when both return true, moreover in case of error _boxes will contain an unitialized box.

#846 should use Hive.deleteBoxFromDisk('boxName'); which handles the delete of unopened box ( see implementation ).

  @override
  Future<void> deleteBoxFromDisk(String name, {String? path}) async {
    var lowerCaseName = name.toLowerCase();
    var box = _boxes[lowerCaseName];
    if (box != null) {
      await box.deleteFromDisk();
    } else {
      await _manager.deleteBox(lowerCaseName, path ?? homePath);
    }
  }

maRci002 · 2022-02-25T10:13:41Z

In nutshell if some one calls Hive.openBox while await newBox.initialize(); is working it is possible to acquire an uninitialized box.

themisir · 2022-02-25T10:32:55Z

Yes that's correct. But there's another issue with your proposed solution. Check #846 for additional details. TLDR if initialization fails db file keeps locked on WindowsOS, so you can't delete or re-open said box.

themisir · 2022-02-25T10:40:35Z

Also when you re-call openBox it will not return uninitialized box because we have some sort of locking mechanism in place that will await openBox calls to same box while initialization completes.

https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L82-L89

themisir · 2022-02-25T10:48:13Z

#846 should use Hive.deleteBoxFromDisk('boxName'); which handles the delete of unopened box ( see implementation ).

It looks like Hive.deleteBoxFromDisk doesn't works in that case because windows thinks file is currently in use as hive doesn't closes opened file on initialization failure.

This is not to say our implementation is flawless. Ofc it's still buggy as someone can call openBox without awaiting and then call box to get instance to uninitialized box object.

Maybe we can try fixing #846 in a different way instead. One way I can think of is closing box on initialization failure..

Let me know what you think about that.

/cc @Jon-Salmon, I would like to know your thoughts too

PS: sorry for comment spamming

maRci002 · 2022-02-25T10:56:16Z

It is possible to return uninitialized box since isOpenBox is before _openingBoxes.containsKey.

https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L75-L89

Run my example code and check output.
Take 2x breakpoint
First:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L79

Second:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/backend/vm/storage_backend_vm.dart#L81

And you can see that the first breakpoint returns the box while initializing it.

themisir · 2022-02-25T11:00:52Z

It is possible to return uninitialized box since isOpenBox is before _openingBoxes.containsKey.

True! Missed that part, sorry. Let's merge your patch and I'll work on fixing #846.

maRci002 · 2022-02-25T11:10:45Z

I think this can be moved before try and make is nullable
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L103

In catch block it can be closed if it's not null
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L116

I think these should be nullable
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/backend/vm/storage_backend_vm.dart#L29-L40

So close close should only close underlying file if it is not null.

This is because open can fail too and we can avoid LateInitializationError if one of them died during BackendManager.open:
https://github.com/hivedb/hive/blob/99212d9f521520cb27cfe5aa2d522729c11d2799/hive/lib/src/hive_impl.dart#L99-L100

maRci002 · 2022-02-25T11:29:00Z

@themisir go ahead and merge. Let me know when you finished #846 fix.

Do not return uninitialized box

04d374b

themisir merged commit 0aa97b8 into isar:master Feb 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not return uninitialized box#916

Do not return uninitialized box#916
themisir merged 1 commit into
isar:masterfrom
maRci002:fix/do-not-return-uninitialized-box

maRci002 commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022 •

edited

Loading

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

maRci002 commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

themisir commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

maRci002 commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

themisir commented Feb 25, 2022 •

edited

Loading