fix: copy names from mmapped memory before closing iterator #22040

davidby-influx · 2021-08-04T01:27:22Z

This fix ensures that memory-mapped files are not released
before pointers into them are copied into heap memory.
MeasurementNamesByExpr() and MeasurementNamesByPredicate() can
cause panics by copying memory from mmapped files that have been
released. The functions they call use iterators to files which
are closed (releasing the mmapped files) before the memory is
safely copied to the heap.

closes #22000

CHANGELOG.md updated with a link to the PR (not the Issue)
Well-formatted commit messages
Rebased/mergeable
Tests pass

This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes #22000

lesam · 2021-08-04T16:06:12Z

tsdb/index.go

+				return
+			}
+
+			ls, lsfi, err := exprToSlice(e.LHS)


lsfi.ToSlice() gives ls right? Why name both?

Because ToSlice() can fail on none-slice-based iterators.

lesam · 2021-08-04T16:09:32Z

tsdb/index.go

 			if e.Op == influxql.OR {
-				return bytesutil.Union(lhs, rhs), nil
+				return newFileMeasurementSliceIterator(bytesutil.Union(ls, rs), mis), nil


Similarly it might be clearer if this read like:

newFileMeasurementSliceIterator(bytesutil.Union(lsfi.UnsafeToSlice(), rsfi.UnsafeToSlice), MeasurementIterators{lsfi, rsfi})

Also would it be clearer to name MeasurementIterators as MeasurementIteratorClosers ?

lesam · 2021-08-04T16:10:51Z

tsdb/index.go

@@ -968,6 +988,46 @@ func (itr *measurementSliceIterator) Next() (name []byte, err error) {
 	return name, nil
 }

+func (itr *measurementSliceIterator) ToSlice() [][]byte {
+	return itr.names


Having both ToSlice and Next on the same object seems prone to accident...

Maybe if it was named RemainingAsSlice or something that suggests it is part of the iteration interface

UnderlyingSlice() is the new name

Hmm, UnderlyingSlice still hints to me that the slice is the whole thing instead of eaten away at by Next, but ok.

lesam · 2021-08-04T16:16:25Z

tsdb/index.go


 	mitr, err := is.measurementIterator()
 	if err != nil {
 		return nil, err
 	} else if mitr == nil {
 		return nil, nil
 	}
-	defer mitr.Close()
+	defer func() {


I've recently seen a pattern using sync.Once to do this sort of thing, and I like it.

E.g.:

// early: once sync.Once{} defer once.Do(mitr.Close) //later var toReturn ... once.Do(func(){ toReturn = newFileMeasurementSliceIterator(names, MeasurementIterators{mitr}), nil }) return toReturn

So you're guaranteed just one of either the close or the delegation of the close succeeds.

This comment doesn't need to be taken for the PR, just an observation.

I think that this pattern won't work here, because the initial defer once.Do(mitr.Close) will be called first, before the close on the toReturn. And that's what is causing the panic; a close on the iterator before the data is read out. But it's a nice pattern for other places.

Hmm - I think it would work, here's a playground example. But not necessary here. https://play.golang.org/p/35qO7UPTOyG

Ah, I see; Once works differently than a similar construct I used in another language; my mistake.

lesam

See comments

lesam · 2021-08-04T16:22:02Z

Mostly naming comments.

lesam · 2021-08-04T19:02:18Z

tsdb/index.go

@@ -1422,24 +1471,24 @@ func (is IndexSet) measurementNamesByExpr(auth query.FineAuthorizer, expr influx
 	case *influxql.ParenExpr:
 		return is.measurementNamesByExpr(auth, e.Expr)
 	default:
-		return nil, fmt.Errorf("Invalid measurement expression %#v", expr)
+		return nil, fmt.Errorf("invalid measurement expression %#v", expr)


lesam · 2021-08-04T19:05:07Z

tsdb/index.go

@@ -949,6 +949,11 @@ func (a MeasurementIterators) Close() (err error) {
 	return err
 }

+type MeasurementSliceIterator interface {


This is much nicer than ToSlice as a free function.

lesam · 2021-08-04T19:18:19Z

Ugh - you hit a new flaky test, I have a PR up to fix: #22038

This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes #22000 (cherry picked from commit a989f8f)

…22058) This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes #22000 (cherry picked from commit a989f8f) Closes #22001

…22059) This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes #22000 (cherry picked from commit a989f8f) closes #22002

…22060) This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes #22000 (cherry picked from commit a989f8f) closes #22003

…ta#22040) This fix ensures that memory-mapped files are not released before pointers into them are copied into heap memory. MeasurementNamesByExpr() and MeasurementNamesByPredicate() can cause panics by copying memory from mmapped files that have been released. The functions they call use iterators to files which are closed (releasing the mmapped files) before the memory is safely copied to the heap. closes influxdata#22000

davidby-influx added kind/bug area/meta queries 1.x team/edge labels Aug 4, 2021

davidby-influx requested a review from lesam August 4, 2021 01:27

davidby-influx self-assigned this Aug 4, 2021

davidby-influx force-pushed the DSB_infor_panic_new branch from c4ca240 to 2171e57 Compare August 4, 2021 01:37

davidby-influx added 2 commits August 3, 2021 18:48

chore: formatting

b6ccacc

davidby-influx force-pushed the DSB_infor_panic_new branch from 4aa6578 to b6ccacc Compare August 4, 2021 01:48

lesam reviewed Aug 4, 2021

View reviewed changes

lesam suggested changes Aug 4, 2021

View reviewed changes

chore: refactor for clarity

ed6039e

davidby-influx requested a review from lesam August 4, 2021 18:42

lesam reviewed Aug 4, 2021

View reviewed changes

lesam approved these changes Aug 4, 2021

View reviewed changes

davidby-influx merged commit a989f8f into master-1.x Aug 4, 2021

davidby-influx deleted the DSB_infor_panic_new branch August 4, 2021 20:16

lesam mentioned this pull request Mar 1, 2022

panic in 1.7.8 while executing localintersect2by2 function #15729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: copy names from mmapped memory before closing iterator #22040

fix: copy names from mmapped memory before closing iterator #22040

davidby-influx commented Aug 4, 2021 •

edited

Loading

lesam Aug 4, 2021

davidby-influx Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

davidby-influx Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

davidby-influx Aug 4, 2021

lesam Aug 4, 2021

davidby-influx Aug 4, 2021

lesam left a comment

lesam commented Aug 4, 2021

lesam Aug 4, 2021

lesam Aug 4, 2021

lesam commented Aug 4, 2021

fix: copy names from mmapped memory before closing iterator #22040

fix: copy names from mmapped memory before closing iterator #22040

Conversation

davidby-influx commented Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesam left a comment

Choose a reason for hiding this comment

lesam commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesam commented Aug 4, 2021

davidby-influx commented Aug 4, 2021 •

edited

Loading