Skip to content

Commit

Permalink
Add some checks before removing directories
Browse files Browse the repository at this point in the history
Fixes #7822

This change first ensures that databases and retention policies exist
before attempting to remove them from the Store. It also adds some
checks in the `DeleteDatabase` and `DeleteRetentionPolicy` to ensure
that maliciously named entries won't remove anything outside of the
configured data directory.
  • Loading branch information
joelegasse committed Jan 12, 2017
1 parent 959a36f commit b19260f
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ The stress tool `influx_stress` will be removed in a subsequent release. We reco
- [#7563](https://github.com/influxdata/influxdb/issues/7563): RP should not allow `INF` or `0` as a shard duration.
- [#7585](https://github.com/influxdata/influxdb/pull/7585): Return Error instead of panic when decoding point values.
- [#7812](https://github.com/influxdata/influxdb/issues/7812): Fix slice out of bounds panic when pruning shard groups. Thanks @vladlopes
- [#7822](https://github.com/influxdata/influxdb/issues/7822): Drop database will delete /influxdb/data directory

## v1.1.1 [2016-12-06]

Expand Down
14 changes: 14 additions & 0 deletions coordinator/statement_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,10 @@ func (e *StatementExecutor) executeDropContinuousQueryStatement(q *influxql.Drop
// It does not return an error if the database was not found on any of
// the nodes, or in the Meta store.
func (e *StatementExecutor) executeDropDatabaseStatement(stmt *influxql.DropDatabaseStatement) error {
if e.MetaClient.Database(stmt.Name) == nil {
return nil
}

// Locally delete the datababse.
if err := e.TSDBStore.DeleteDatabase(stmt.Name); err != nil {
return err
Expand Down Expand Up @@ -348,6 +352,16 @@ func (e *StatementExecutor) executeDropShardStatement(stmt *influxql.DropShardSt
}

func (e *StatementExecutor) executeDropRetentionPolicyStatement(stmt *influxql.DropRetentionPolicyStatement) error {

dbi := e.MetaClient.Database(stmt.Database)
if dbi == nil {
return nil
}

if dbi.RetentionPolicy(stmt.Name) == nil {
return nil
}

// Locally drop the retention policy.
if err := e.TSDBStore.DeleteRetentionPolicy(stmt.Database, stmt.Name); err != nil {
return err
Expand Down
26 changes: 25 additions & 1 deletion tsdb/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,11 @@ func (s *Store) DeleteShard(shardID uint64) error {
// DeleteDatabase will close all shards associated with a database and remove the directory and files from disk.
func (s *Store) DeleteDatabase(name string) error {
s.mu.RLock()
if s.databaseIndexes[name] == nil {
s.mu.RUnlock()
// no files locally, so nothing to do
return nil
}
shards := s.filterShards(func(sh *Shard) bool {
return sh.database == name
})
Expand All @@ -402,7 +407,15 @@ func (s *Store) DeleteDatabase(name string) error {
return err
}

if err := os.RemoveAll(filepath.Join(s.path, name)); err != nil {
dbPath := filepath.Clean(filepath.Join(s.path, name))

// extra sanity check to make sure that even if someone named their database "../.."
// that we don't delete everything because of it, they'll just have extra files forever
if filepath.Clean(s.path) != filepath.Dir(dbPath) {
return fmt.Errorf("invalid database directory location for database '%s': %s", name, dbPath)
}

if err := os.RemoveAll(dbPath); err != nil {
return err
}
if err := os.RemoveAll(filepath.Join(s.EngineOptions.Config.WALDir, name)); err != nil {
Expand All @@ -424,6 +437,10 @@ func (s *Store) DeleteDatabase(name string) error {
// both the DB and WAL, and remove all shard files from disk.
func (s *Store) DeleteRetentionPolicy(database, name string) error {
s.mu.RLock()
if s.databaseIndexes[database] == nil {
// unknown database, nothing to do
return nil
}
shards := s.filterShards(func(sh *Shard) bool {
return sh.database == database && sh.retentionPolicy == name
})
Expand All @@ -441,6 +458,13 @@ func (s *Store) DeleteRetentionPolicy(database, name string) error {
return err
}

rpPath := filepath.Clean(filepath.Join(s.path, database, name))

// ensure Store's path is the grandparent of the retention policy
if filepath.Clean(s.path) != filepath.Dir(filepath.Dir(rpPath)) {
return fmt.Errorf("invalid path for database '%s', retention policy '%s': %s", database, name, rpPath)
}

// Remove the rentention policy folder.
if err := os.RemoveAll(filepath.Join(s.path, database, name)); err != nil {
return err
Expand Down

0 comments on commit b19260f

Please sign in to comment.