planner/core: fix privilege check for update #10281

tiancaiamao · 2019-04-26T15:06:34Z

What problem does this PR solve?

create database service;
create database report;
CREATE TABLE service.t1 (
  id int(11) DEFAULT NULL,
  a bigint(20) NOT NULL,
  b text DEFAULT NULL,
  PRIMARY KEY (a)
);
CREATE TABLE report.t2 (
  a bigint(20) DEFAULT NULL,
  c bigint(20) NOT NULL
);
create user 'test'@'%';
grant all privileges on service.* to test;
grant all privileges on report.* to test;

Login as test, run this update statement:

mysql> update service.t1 s,
    -> report.t2 t
    -> set s.a = t.a
    -> WHERE
    -> s.a = t.a
    -> and t.c >=  1 and t.c <= 10000
    -> and s.b !='xx';
ERROR 1105 (HY000): privilege check fail

What is changed and how it works?

One of the visitInfo record is

db=""
table="t1"
user="test"

It doesn't match any privilege record in the cache, so privilege check fail.

The bug is, db should not be "" here.

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

Related change

Need to cherry-pick to the release branch

tiancaiamao · 2019-04-26T15:06:49Z

@zimulala @winkyao

codecov · 2019-04-26T15:29:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4cf3630). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #10281   +/-   ##
===========================================
  Coverage          ?   77.6715%           
===========================================
  Files             ?        411           
  Lines             ?      85447           
  Branches          ?          0           
===========================================
  Hits              ?      66368           
  Misses            ?      14118           
  Partials          ?       4961

shenli · 2019-04-28T02:29:18Z

planner/core/logical_plan_builder.go

@@ -157,7 +157,9 @@ func (b *PlanBuilder) buildResultSetNode(node ast.ResultSetNode) (p LogicalPlan,
 			col.OrigTblName = col.TblName
 			if x.AsName.L != "" {
 				col.TblName = x.AsName
-				col.DBName = model.NewCIStr("")
+				if b.ctx.GetSessionVars().CurrentDB == col.DBName.L {


Could you add a comment here? Why should we check the currentDB name and set DBName to empty string?

We can put an example here.

I don't know why we set col.DBName = model.NewCIStr("") here @zimulala
I should remove this line ... but it would make explain result (column name) change and affect many rows in the test

OK, I'll remove this line... that's a better fix, anyway

@tiancaiamao
I am not sure, I just changed the name of DBName. This line of code was originally written like this.
PTAL @XuHuaiyu

I guess the reason may be this:
When a table has an alias name, this "virtual table" does not belong to any database, we thought the DBName should be blank.
But I think it seems ok to keep the DBName here.

tiancaiamao · 2019-04-28T13:40:01Z

planner/core/logical_plan_builder.go

@@ -157,7 +157,6 @@ func (b *PlanBuilder) buildResultSetNode(node ast.ResultSetNode) (p LogicalPlan,
 			col.OrigTblName = col.TblName
 			if x.AsName.L != "" {
 				col.TblName = x.AsName
-				col.DBName = model.NewCIStr("")


This line is the real change in this PR

why add an empty database name can fix the issue?

One of the visitInfo record is

db="" table="t1" user="test"

It doesn't match any privilege record in the cache, so privilege check fail.

@zz-jason

The db name is not set correctly...

The db name is set to "", and we should not set it to ""

tiancaiamao · 2019-04-28T13:40:37Z

PTAL @winkyao @zimulala @shenli

winkyao

LGTM

zimulala · 2019-04-30T02:47:42Z

cmd/explaintest/r/explain_complex_stats.result

    ├─TableReader_41	428.32	root	data:Selection_40
-    │ └─Selection_40	428.32	cop	eq(dt.bm, 0), eq(dt.pt, "ios"), gt(dt.t, 1478185592), not(isnull(dt.dic))
+    │ └─Selection_40	428.32	cop	eq(test.dt.bm, 0), eq(test.dt.pt, "ios"), gt(test.dt.t, 1478185592), not(isnull(test.dt.dic))
    │   └─TableScan_39	2000.00	cop	table:dt, range:[0,+inf], keep order:false


Is it dt is alias name? If it's true, it is called dt here, but it's called test.dt on line 165. Is this a bit confused?

You mean we should use test.dt in line 166? Currently, we do not print database name for PhysicalTableScan.

func (p *PhysicalTableScan) ExplainInfo() string { buffer := bytes.NewBufferString("") tblName := p.Table.Name.O if p.TableAsName != nil && p.TableAsName.O != "" { tblName = p.TableAsName.O } fmt.Fprintf(buffer, "table:%s", tblName) ... }

If needed, we can print DBName as well, since it is recorded in PhysicalTableScan.

@eurekaka
No, I want to say if this situation will make you feel troubled.

Oh I think it is not bothering since we merely meet queries where 2 tables share same name but from different databases.

zimulala

LGTM

zimulala · 2019-04-30T03:31:45Z

/run-all-tests

planner/core: fix privilege check for update

dc7322f

tiancaiamao added the component/privilege label Apr 26, 2019

address comment

985a0c5

shenli reviewed Apr 28, 2019

View reviewed changes

better fix

f1160d0

tiancaiamao commented Apr 28, 2019

View reviewed changes

winkyao reviewed Apr 29, 2019

View reviewed changes

Merge branch 'master' into privilege-update

32969af

zimulala reviewed Apr 30, 2019

View reviewed changes

zimulala approved these changes Apr 30, 2019

View reviewed changes

zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 30, 2019

Merge branch 'master' into privilege-update

e9b22ac

tiancaiamao added the status/all tests passed label Apr 30, 2019

tiancaiamao merged commit efe9b6a into pingcap:master Apr 30, 2019

tiancaiamao deleted the privilege-update branch April 30, 2019 05:06

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Apr 30, 2019

planner/core: fix privilege check for update (pingcap#10281)

c3c467d

tiancaiamao mentioned this pull request Apr 30, 2019

planner/core: fix privilege check for update (#10281) #10326

Merged

jackysp pushed a commit that referenced this pull request Apr 30, 2019

planner/core: fix privilege check for update (#10281) (#10326)

c089bb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner/core: fix privilege check for update #10281

planner/core: fix privilege check for update #10281

tiancaiamao commented Apr 26, 2019 •

edited

Loading

tiancaiamao commented Apr 26, 2019

codecov bot commented Apr 26, 2019 •

edited

Loading

shenli Apr 28, 2019

winkyao Apr 28, 2019

tiancaiamao Apr 28, 2019

tiancaiamao Apr 28, 2019

zimulala Apr 30, 2019

XuHuaiyu Apr 30, 2019

tiancaiamao Apr 28, 2019

zz-jason Apr 30, 2019

tiancaiamao Apr 30, 2019

tiancaiamao Apr 30, 2019 •

edited

Loading

tiancaiamao Apr 30, 2019

tiancaiamao commented Apr 28, 2019

winkyao left a comment

zimulala Apr 30, 2019 •

edited

Loading

zimulala Apr 30, 2019

eurekaka Apr 30, 2019

zimulala Apr 30, 2019

eurekaka Apr 30, 2019

zimulala Apr 30, 2019

zimulala left a comment

zimulala commented Apr 30, 2019

planner/core: fix privilege check for update #10281

planner/core: fix privilege check for update #10281

Conversation

tiancaiamao commented Apr 26, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Apr 26, 2019

codecov bot commented Apr 26, 2019 • edited Loading

Codecov Report

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

tiancaiamao Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Apr 28, 2019

winkyao left a comment

Choose a reason for hiding this comment

zimulala Apr 30, 2019 • 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

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Apr 30, 2019

tiancaiamao commented Apr 26, 2019 •

edited

Loading

codecov bot commented Apr 26, 2019 •

edited

Loading

tiancaiamao Apr 30, 2019 •

edited

Loading

zimulala Apr 30, 2019 •

edited

Loading