Skip to content

Commit

Permalink
Refactored the code backing DBKEY concatenation for views. Re-attempt…
Browse files Browse the repository at this point in the history 
…ed to fix CORE-4985: Non-privileged user can implicitly count records in a restricted table.
  • Loading branch information
@dyemanov
dyemanov committed Jun 15, 2020
1 parent 4782e44 commit 82b2b21
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 180 deletions.
8 changes: 1 addition & 7 deletions src/dsql/BoolNodes.cpp
View file
Expand Up @@ -1727,13 +1727,8 @@ BoolExprNode* RseBoolNode::pass1(thread_db* tdbb, CompilerScratch* csb)
boolean->nodFlags |= FLAG_RESIDUAL | (deoptimize ? FLAG_DEOPTIMIZE : 0);
}
}
// fall into

case blr_any:
case blr_exists:
case blr_unique:
rse->ignoreDbKey(tdbb, csb);
break;
break;
}

return BoolExprNode::pass1(tdbb, csb);
Expand Down Expand Up @@ -1883,7 +1878,6 @@ BoolExprNode* RseBoolNode::convertNeqAllToNotAny(thread_db* tdbb, CompilerScratc
andNode->arg2 = rseBoolNode;

RseNode* newInnerRse = innerRse->clone(csb->csb_pool);
newInnerRse->ignoreDbKey(tdbb, csb);

rseBoolNode = FB_NEW_POOL(csb->csb_pool) RseBoolNode(csb->csb_pool, blr_any);
rseBoolNode->rse = newInnerRse;
Expand Down
79 changes: 33 additions & 46 deletions src/dsql/ExprNodes.cpp
View file
Expand Up @@ -70,72 +70,60 @@ using namespace Jrd;

namespace
{
// Expand DBKEY for view
void expandViewNodes(thread_db* tdbb, CompilerScratch* csb, StreamType stream,
ValueExprNodeStack& stack, UCHAR blrOp)
{
const CompilerScratch::csb_repeat* const csb_tail = &csb->csb_rpt[stream];

// If the stream's dbkey should be ignored, do so

if (csb_tail->csb_flags & csb_no_dbkey)
return;

// If the stream references a view, follow map

const StreamType* map = csb_tail->csb_map;
if (map)
{
++map;

while (*map)
expandViewNodes(tdbb, csb, *map++, stack, blrOp);

return;
}

// Relation is primitive - make DBKEY node

if (csb_tail->csb_relation)
{
RecordKeyNode* node = FB_NEW_POOL(csb->csb_pool) RecordKeyNode(csb->csb_pool, blrOp);
node->recStream = stream;
stack.push(node);
}
}

// Try to expand the given stream. If it's a view reference, collect its base streams
// (the ones directly residing in the FROM clause) and recurse.
void expandViewStreams(CompilerScratch* csb, StreamType stream, SortedStreamList& streams)
void expandViewStreams(CompilerScratch* csb, StreamType baseStream, SortedStreamList& streams)
{
const CompilerScratch::csb_repeat* const csb_tail = &csb->csb_rpt[stream];
const auto csb_tail = &csb->csb_rpt[baseStream];

const RseNode* const rse =
csb_tail->csb_relation ? csb_tail->csb_relation->rel_view_rse : NULL;

// If we have a view, collect its base streams and remap/expand them.
// If we have a view, collect its base streams and remap/expand them

if (rse)
{
const StreamType* const map = csb_tail->csb_map;
const auto map = csb_tail->csb_map;
fb_assert(map);

StreamList viewStreams;
rse->computeRseStreams(viewStreams);

for (StreamType* iter = viewStreams.begin(); iter != viewStreams.end(); ++iter)
for (auto stream : viewStreams)
{
// Remap stream and expand it recursively
expandViewStreams(csb, map[*iter], streams);
expandViewStreams(csb, map[stream], streams);
}

return;
}

// Otherwise, just add the current stream to the list.
// Otherwise, just add the current stream to the list

if (!streams.exist(baseStream))
streams.add(baseStream);
}

if (!streams.exist(stream))
streams.add(stream);
// Expand DBKEY for view
void expandViewNodes(CompilerScratch* csb, StreamType baseStream,
ValueExprNodeStack& stack, UCHAR blrOp)
{
SortedStreamList viewStreams;
expandViewStreams(csb, baseStream, viewStreams);

for (auto stream : viewStreams)
{
const auto csb_tail = &csb->csb_rpt[stream];

// If relation is primitive, make DBKEY node

if (csb_tail->csb_relation)
{
const auto node = FB_NEW_POOL(csb->csb_pool) RecordKeyNode(csb->csb_pool, blrOp);
node->recStream = stream;
stack.push(node);
}
}
}

// Look at all RSEs which are lower in scope than the RSE which this field is
Expand Down Expand Up @@ -6581,7 +6569,7 @@ ValueExprNode* FieldNode::pass1(thread_db* tdbb, CompilerScratch* csb)
// the nodes in the subtree are involved in a validation
// clause only, the subtree is a validate_subtree in our notation.

SLONG ssRelationId = tail->csb_view ?
const SLONG ssRelationId = tail->csb_view ?
tail->csb_view->rel_id : (csb->csb_view ? csb->csb_view->rel_id : 0);

if (tail->csb_flags & csb_modify)
Expand Down Expand Up @@ -10029,7 +10017,7 @@ ValueExprNode* RecordKeyNode::pass1(thread_db* tdbb, CompilerScratch* csb)
return this;

ValueExprNodeStack stack;
expandViewNodes(tdbb, csb, recStream, stack, blrOp);
expandViewNodes(csb, recStream, stack, blrOp);

#ifdef CMP_DEBUG
csb->dump("expand RecordKeyNode: %d\n", recStream);
Expand Down Expand Up @@ -11163,7 +11151,6 @@ bool SubQueryNode::sameAs(CompilerScratch* /*csb*/, const ExprNode* /*other*/, b

ValueExprNode* SubQueryNode::pass1(thread_db* tdbb, CompilerScratch* csb)
{
rse->ignoreDbKey(tdbb, csb);
doPass1(tdbb, csb, rse.getAddress());

csb->csb_current_nodes.push(rse.getObject());
Expand Down
1 change: 0 additions & 1 deletion src/dsql/Nodes.h
View file
Expand Up @@ -1113,7 +1113,6 @@ class RecordSourceNode : public ExprNode
}

virtual RecordSourceNode* copy(thread_db* tdbb, NodeCopier& copier) const = 0;
virtual void ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const = 0;
virtual RecordSourceNode* pass1(thread_db* tdbb, CompilerScratch* csb) = 0;
virtual void pass1Source(thread_db* tdbb, CompilerScratch* csb, RseNode* rse,
BoolExprNode** boolean, RecordSourceNodeStack& stack) = 0;
Expand Down
7 changes: 6 additions & 1 deletion src/dsql/StmtNodes.cpp
View file
Expand Up @@ -5016,7 +5016,12 @@ void ForNode::genBlr(DsqlCompilerScratch* dsqlScratch)
StmtNode* ForNode::pass1(thread_db* tdbb, CompilerScratch* csb)
{
doPass1(tdbb, csb, stall.getAddress());
doPass1(tdbb, csb, rse.getAddress());

{ // scope
AutoSetRestore<bool> autoImplicitCursor(&csb->csb_implicit_cursor, forUpdate);
doPass1(tdbb, csb, rse.getAddress());
}

doPass1(tdbb, csb, statement.getAddress());
return this;
}
Expand Down
104 changes: 6 additions & 98 deletions src/jrd/RecordSourceNodes.cpp
View file
Expand Up @@ -612,47 +612,6 @@ RelationSourceNode* RelationSourceNode::copy(thread_db* tdbb, NodeCopier& copier
element->csb_view = newSource->view;
element->csb_view_stream = copier.remap[0];

/*
If there was a parent stream no., then copy the flags
from that stream to its children streams. (Bug 10164/10166)
For e.g. consider a view V1 with 2 streams:
stream #1 from table T1
stream #2 from table T2
consider a procedure P1 with 2 streams:
stream #1 from table X
stream #2 from view V1
During pass1 of procedure request, the engine tries to expand
all the views into their base tables. It creates a compiler
scratch block which initially looks like this:
stream 1 -------- X
stream 2 -------- V1
while expanding V1 the engine calls copy() with nod_relation.
A new stream 3 is created. Now the CompilerScratch looks like:
stream 1 -------- X
stream 2 -------- V1 map [2,3,4]
stream 3 -------- T1
stream 4 -------- T2
After T1 stream has been created the flags are copied from
stream #1 because V1's definition said the original stream
number for T1 was 1. However since its being merged with
the procedure request, stream #1 belongs to a different table.
The flags should be copied from stream 2 i.e. V1.
Since we didn't do this properly before, V1's children got
tagged with whatever flags X possesed leading to various
errors.
*/

copier.csb->inheritViewFlags(newSource->stream, csb_no_dbkey);

if (alias.hasData())
{
element->csb_alias = FB_NEW_POOL(*tdbb->getDefaultPool())
Expand All @@ -662,20 +621,20 @@ RelationSourceNode* RelationSourceNode::copy(thread_db* tdbb, NodeCopier& copier
return newSource;
}

void RelationSourceNode::ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const
RecordSourceNode* RelationSourceNode::pass1(thread_db* tdbb, CompilerScratch* csb)
{
csb->csb_rpt[stream].csb_flags |= csb_no_dbkey;

const CompilerScratch::csb_repeat* const tail = &csb->csb_rpt[stream];
const jrd_rel* const relation = tail->csb_relation;
const auto tail = &csb->csb_rpt[stream];
const auto relation = tail->csb_relation;

if (relation)
if (relation && !csb->csb_implicit_cursor)
{
const SLONG ssRelationId = tail->csb_view ? tail->csb_view->rel_id :
view ? view->rel_id : csb->csb_view ? csb->csb_view->rel_id : 0;
CMP_post_access(tdbb, csb, relation->rel_security_name, ssRelationId,
SCL_select, SCL_object_table, relation->rel_name);
}

return this;
}

void RelationSourceNode::pass1Source(thread_db* tdbb, CompilerScratch* csb, RseNode* rse,
Expand Down Expand Up @@ -1132,11 +1091,6 @@ ProcedureSourceNode* ProcedureSourceNode::copy(thread_db* tdbb, NodeCopier& copi
element->csb_view = newSource->view;
element->csb_view_stream = copier.remap[0];

// dimitr: I doubt we need to inherit this flag for procedures.
// They don't have a DBKEY to be concatenated.
// Neither they have a stream map to be expanded.
copier.csb->inheritViewFlags(newSource->stream, csb_no_dbkey);

if (alias.hasData())
{
element->csb_alias = FB_NEW_POOL(*tdbb->getDefaultPool())
Expand Down Expand Up @@ -1490,8 +1444,6 @@ AggregateSourceNode* AggregateSourceNode::copy(thread_db* tdbb, NodeCopier& copi
copier.remap[stream] = newSource->stream;
CMP_csb_element(copier.csb, newSource->stream);

copier.csb->inheritViewFlags(newSource->stream, csb_no_dbkey);

newSource->rse = rse->copy(tdbb, copier);
if (group)
newSource->group = group->copy(tdbb, copier);
Expand All @@ -1500,16 +1452,8 @@ AggregateSourceNode* AggregateSourceNode::copy(thread_db* tdbb, NodeCopier& copi
return newSource;
}

void AggregateSourceNode::ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const
{
rse->ignoreDbKey(tdbb, csb);
}

RecordSourceNode* AggregateSourceNode::pass1(thread_db* tdbb, CompilerScratch* csb)
{
csb->csb_rpt[stream].csb_flags |= csb_no_dbkey;
rse->ignoreDbKey(tdbb, csb);

doPass1(tdbb, csb, rse.getAddress());
doPass1(tdbb, csb, map.getAddress());
doPass1(tdbb, csb, group.getAddress());
Expand Down Expand Up @@ -1813,15 +1757,11 @@ UnionSourceNode* UnionSourceNode::copy(thread_db* tdbb, NodeCopier& copier) cons
copier.remap[stream] = newSource->stream;
CMP_csb_element(copier.csb, newSource->stream);

copier.csb->inheritViewFlags(newSource->stream, csb_no_dbkey);

if (newSource->recursive)
{
newSource->mapStream = copier.csb->nextStream();
copier.remap[mapStream] = newSource->mapStream;
CMP_csb_element(copier.csb, newSource->mapStream);

copier.csb->inheritViewFlags(newSource->mapStream, csb_no_dbkey);
}

const NestConst<RseNode>* ptr = clauses.begin();
Expand All @@ -1836,14 +1776,6 @@ UnionSourceNode* UnionSourceNode::copy(thread_db* tdbb, NodeCopier& copier) cons
return newSource;
}

void UnionSourceNode::ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const
{
const NestConst<RseNode>* ptr = clauses.begin();

for (const NestConst<RseNode>* const end = clauses.end(); ptr != end; ++ptr)
(*ptr)->ignoreDbKey(tdbb, csb);
}

void UnionSourceNode::pass1Source(thread_db* tdbb, CompilerScratch* csb, RseNode* /*rse*/,
BoolExprNode** /*boolean*/, RecordSourceNodeStack& stack)
{
Expand Down Expand Up @@ -2232,8 +2164,6 @@ WindowSourceNode* WindowSourceNode::copy(thread_db* tdbb, NodeCopier& copier) co
copier.remap[inputWindow->stream] = copyWindow.stream;
CMP_csb_element(copier.csb, copyWindow.stream);

copier.csb->inheritViewFlags(copyWindow.stream, csb_no_dbkey);

if (inputWindow->group)
copyWindow.group = inputWindow->group->copy(tdbb, copier);

Expand All @@ -2253,21 +2183,8 @@ WindowSourceNode* WindowSourceNode::copy(thread_db* tdbb, NodeCopier& copier) co
return newSource;
}

void WindowSourceNode::ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const
{
rse->ignoreDbKey(tdbb, csb);
}

RecordSourceNode* WindowSourceNode::pass1(thread_db* tdbb, CompilerScratch* csb)
{
for (ObjectsArray<Window>::iterator window = windows.begin();
window != windows.end();
++window)
{
csb->csb_rpt[window->stream].csb_flags |= csb_no_dbkey;
}

rse->ignoreDbKey(tdbb, csb);
doPass1(tdbb, csb, rse.getAddress());

for (ObjectsArray<Window>::iterator window = windows.begin();
Expand Down Expand Up @@ -2810,15 +2727,6 @@ RseNode* RseNode::copy(thread_db* tdbb, NodeCopier& copier) const
return newSource;
}

// For each relation or aggregate in the RseNode, mark it as not having a dbkey.
void RseNode::ignoreDbKey(thread_db* tdbb, CompilerScratch* csb) const
{
const NestConst<RecordSourceNode>* ptr = rse_relations.begin();

for (const NestConst<RecordSourceNode>* const end = rse_relations.end(); ptr != end; ++ptr)
(*ptr)->ignoreDbKey(tdbb, csb);
}

// Process a record select expression during pass 1 of compilation.
// Mostly this involves expanding views.
RseNode* RseNode::pass1(thread_db* tdbb, CompilerScratch* csb)
Expand Down

0 comments on commit 82b2b21

Please sign in to comment.