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

block-size not sanitized, set it larger than 4GB by mistake may cause data corruption #5486

Closed
zhangjinpeng87 opened this issue Jun 19, 2019 · 3 comments

Comments

@zhangjinpeng87
Copy link
Contributor

Note: Please use Issues only for bug reports. For questions, discussions, feature requests, etc. post to dev group: https://www.facebook.com/groups/rocksdb.dev

Expected behavior

When set block-size larger than 4GB, it should be forbidden.

Actual behavior

When i set block-size as 8GB, RocksDB started up and encountered "Corruption: bad entry in block" error after some compaction.

Steps to reproduce the behavior

  1. Load data
  2. Set block-size = 8GB
  3. Compact range
  4. Read data

@siddontang @yiwu-arbug

@miasantreble
Copy link
Contributor

I wrote a test but was unable to reproduce the failure, do you mind taking a look?

TEST_F(DBTest, LargeBlockSizeTest) {
  Options options = CurrentOptions();
  CreateAndReopenWithCF({"pikachu"}, options);
  ASSERT_OK(Put(0, "foo", "bar"));
  BlockBasedTableOptions table_options;
  table_options.block_size = 8LL*1024*1024*1024LL;
  options.table_factory.reset(NewBlockBasedTableFactory(table_options));
  ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
  ASSERT_OK(Put(0, "foo1", "bar1"));
  db_->CompactRange(CompactRangeOptions(), nullptr, nullptr);
  ASSERT_EQ("bar", Get(0, "foo"));
  ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
  ASSERT_EQ("bar1", Get(0, "foo1"));
}

@yiwu-arbug
Copy link
Contributor

@miasantreble try having two >4GB size values in a data block?

@siying
Copy link
Contributor

siying commented Jun 19, 2019

@miasantreble as what @yiwu-arbug said, our restart offset is encoded as fixed uint32. If there is only one key in the block, the offset of the key is 0. But if there are multiple keys, the offset of the key might excceeds max of uint32_t. There is also a a keys per restart index setting, so that you might want to have many keys, or tune the restart index to be 1 to reproduce it.

But clearly block size over max uint32_t won't work.

facebook-github-bot pushed a commit that referenced this issue Jun 20, 2019
Summary:
`Block::restart_index_`, `Block::restarts_`, and `Block::current_` are defined as uint32_t but  `BlockBasedTableOptions::block_size` is defined as a size_t so user might see corruption as in #5486.
This PR adds a check in `BlockBasedTableFactory::SanitizeOptions` to disallow such configurations.
yiwu-arbug
Pull Request resolved: #5492

Differential Revision: D15914047

Pulled By: miasantreble

fbshipit-source-id: c943f153d967e15aee7f2795730ab8259e2be201
merryChris pushed a commit to merryChris/rocksdb that referenced this issue Nov 18, 2019
Summary:
`Block::restart_index_`, `Block::restarts_`, and `Block::current_` are defined as uint32_t but  `BlockBasedTableOptions::block_size` is defined as a size_t so user might see corruption as in facebook#5486.
This PR adds a check in `BlockBasedTableFactory::SanitizeOptions` to disallow such configurations.
yiwu-arbug
Pull Request resolved: facebook#5492

Differential Revision: D15914047

Pulled By: miasantreble

fbshipit-source-id: c943f153d967e15aee7f2795730ab8259e2be201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants