Skip to content

storage: fix null pointer dereference in AdminMerge#38418

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
jeffrey-xiao:fix-admin-merge
Jun 26, 2019
Merged

storage: fix null pointer dereference in AdminMerge#38418
craig[bot] merged 1 commit intocockroachdb:masterfrom
jeffrey-xiao:fix-admin-merge

Conversation

@jeffrey-xiao
Copy link
Copy Markdown
Contributor

@jeffrey-xiao jeffrey-xiao commented Jun 25, 2019

Fixes #38427.

I noticed that TestRepartitioning was failing under stress on master with the following error:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x20894f6]

...

        /usr/lib/go-1.12/src/runtime/panic.go:522 +0x1b5
github.com/cockroachdb/cockroach/pkg/storage.(*Replica).AdminMerge.func1(0xc00441e750, 0xc002a83e60, 0xc00020a880)
        /home/cockroach/go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_command.go:536 +0x226

...

The logic for retrieving the RHS descriptor before and after #38302 is not the same in the case where rightDesc does not exist. This PR changes it so the behavior is consistent.

It seems possible that the following branch can evaluate true, but I might be missing something here.

// Verify that the two ranges are mergeable.
if !bytes.Equal(origLeftDesc.EndKey, rightDesc.StartKey) {
  // Should never happen, but just in case.
  return errors.Errorf("ranges are not adjacent; %s != %s", origLeftDesc.EndKey, rightDesc.StartKey)
}

@jeffrey-xiao jeffrey-xiao requested review from a team, ajwerner, danhhz and nvb June 25, 2019 20:09
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)


pkg/storage/replica_command.go, line 534 at r1 (raw file):

			return err
		}
		if dbRightDescKV.Value != nil {

How about:

if err := dbRighDescKV.ValueProto(&rightDesc); err != nil {
    return err
}

This will better mirror the previous behavior.

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)


pkg/storage/replica_command.go, line 534 at r1 (raw file):

Previously, ajwerner wrote…

How about:

if err := dbRighDescKV.ValueProto(&rightDesc); err != nil {
    return err
}

This will better mirror the previous behavior.

s/Righ/Right/

Copy link
Copy Markdown
Contributor

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @danhhz)


pkg/storage/replica_command.go, line 534 at r1 (raw file):

Previously, ajwerner wrote…

s/Righ/Right/

Yeah, ValueProto looks like the right approach.

Copy link
Copy Markdown
Contributor Author

@jeffrey-xiao jeffrey-xiao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @danhhz)


pkg/storage/replica_command.go, line 534 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, ValueProto looks like the right approach.

Done.

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @danhhz)

@jeffrey-xiao
Copy link
Copy Markdown
Contributor Author

TFTRs!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 26, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Jun 26, 2019
38211: changefeedccl: remove deprecated poller r=tbg a=danhhz

We switched the default to push-based rangefeeds in 19.1. This removes
the old pull-based poller fallback entirely.

Details of the removal:
- The relevant code is removed
- Several poller-related hacks are removed
- The changefeed.run.push.enabled telemetry metric is removed
- The changefeed.push.enabled cluster setting is removed
- The poller subtest is removed from each changefeedccl test
- The cdc/poller roachtest is skipped on 19.2+
- TestValidations is removed, it's redundant with the much better
  quality TestChangefeedNemeses

Note that the table history still does some polling, but switching this
to RangeFeed will cause an unacceptable increase in the commit-to-emit
latency of rows. This bit of polling will be removed as part of #36289.
This commit also leaves the structure of the changefeed code mostly
unchanged. There is an opportunity for cleanup here, but this also will
wait for after #36289.

Closes #36914

Release note: None

38418: storage: fix null pointer dereference in AdminMerge r=jeffrey-xiao a=jeffrey-xiao

Fixes #38427.

I noticed that `TestRepartitioning` was failing under stress on master with the following error:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x20894f6]

...

        /usr/lib/go-1.12/src/runtime/panic.go:522 +0x1b5
github.com/cockroachdb/cockroach/pkg/storage.(*Replica).AdminMerge.func1(0xc00441e750, 0xc002a83e60, 0xc00020a880)
        /home/cockroach/go/src/github.com/cockroachdb/cockroach/pkg/storage/replica_command.go:536 +0x226

...
```

The logic for retrieving the RHS descriptor before and after #38302 is not the same in the case where `rightDesc` does not exist. This PR changes it so the behavior is consistent.

It seems possible that the following branch can evaluate true, but I might be missing something here.
```go
// Verify that the two ranges are mergeable.
if !bytes.Equal(origLeftDesc.EndKey, rightDesc.StartKey) {
  // Should never happen, but just in case.
  return errors.Errorf("ranges are not adjacent; %s != %s", origLeftDesc.EndKey, rightDesc.StartKey)
}
```


38435: sql: Fixing interleave check for loose index scan r=rohany a=rohany

Fixing the interleave check for the loose index scan while interleave support is in progress.

Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com>
Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 26, 2019

Build succeeded

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

Successfully merging this pull request may close these issues.

teamcity: failed test: TestRepartitioning

4 participants