Skip to content

Commit

Permalink
This should fix #7817: Memory leak is possible for UDF array arguments
Browse files Browse the repository at this point in the history
  • Loading branch information
@dyemanov
dyemanov committed Oct 31, 2023
1 parent 5e0cfe3 commit 749bcc0
Showing 1 changed file with 28 additions and 36 deletions.
64 changes: 28 additions & 36 deletions src/jrd/fun.epp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ enum UdfError
static SSHORT blob_get_segment(blb*, UCHAR*, USHORT, USHORT*);
static void blob_put_segment(blb*, const UCHAR*, USHORT);
static SLONG blob_lseek(blb*, USHORT, SLONG);
static SLONG get_scalar_array(const Parameter*, DSC*, scalar_array_desc*, UCharStack&);
static ULONG get_scalar_array(thread_db* tdbb, const Parameter*, DSC*,
scalar_array_desc*, UCharStack&);
static void invoke(thread_db* tdbb,
const Function* function,
const Parameter* return_ptr,
Expand Down Expand Up @@ -436,8 +437,7 @@ void FUN_evaluate(thread_db* tdbb, const Function* function, const NestValueArra

temp_desc = parameter->prm_desc;
temp_desc.dsc_address = temp_ptr;
// CVC: There's a theoretical possibility of overflowing "length" here.
USHORT length = FB_ALIGN(temp_desc.dsc_length, FB_DOUBLE_ALIGN);
ULONG length = FB_ALIGN(temp_desc.dsc_length, FB_DOUBLE_ALIGN);

// If we've got a null argument, just pass zeros (got any better ideas?)

Expand All @@ -446,7 +446,7 @@ void FUN_evaluate(thread_db* tdbb, const Function* function, const NestValueArra
if (parameter->prm_fun_mechanism == FUN_value)
{
UCHAR* p = (UCHAR *) arg_ptr;
MOVE_CLEAR(p, (SLONG) length);
MOVE_CLEAR(p, length);
p += length;
arg_ptr = reinterpret_cast<UDF_ARG*>(p);
continue;
Expand All @@ -458,7 +458,7 @@ void FUN_evaluate(thread_db* tdbb, const Function* function, const NestValueArra
}

if (parameter->prm_fun_mechanism != FUN_ref_with_null)
MOVE_CLEAR(temp_ptr, (SLONG) length);
MOVE_CLEAR(temp_ptr, length);
else
{
// Probably for arrays and blobs it's better to preserve the
Expand All @@ -468,7 +468,7 @@ void FUN_evaluate(thread_db* tdbb, const Function* function, const NestValueArra
case dtype_quad:
case dtype_array:
case dtype_blob:
MOVE_CLEAR(temp_ptr, (SLONG) length);
MOVE_CLEAR(temp_ptr, length);
break;
default: // FUN_ref_with_null, non-blob, non-array: we send null pointer.
*arg_ptr++ = 0;
Expand All @@ -478,7 +478,8 @@ void FUN_evaluate(thread_db* tdbb, const Function* function, const NestValueArra
}
else if (parameter->prm_fun_mechanism == FUN_scalar_array)
{
length = get_scalar_array(parameter, input, (scalar_array_desc*)temp_ptr, array_stack);
length = get_scalar_array(tdbb, parameter, input, (scalar_array_desc*) temp_ptr,
array_stack);
}
else
{
Expand Down Expand Up @@ -975,10 +976,11 @@ static SSHORT blob_get_segment(blb* blob, UCHAR* buffer, USHORT length, USHORT*
}


static SLONG get_scalar_array(const Parameter* arg,
DSC* value,
scalar_array_desc* scalar_desc,
UCharStack& stack)
static ULONG get_scalar_array(thread_db* tdbb,
const Parameter* arg,
DSC* value,
scalar_array_desc* scalar_desc,
UCharStack& stack)
{
/**************************************
*
Expand All @@ -992,17 +994,16 @@ static SLONG get_scalar_array(const Parameter* arg,
* Return length of array desc.
*
**************************************/
thread_db* tdbb = JRD_get_thread_data();
MemoryPool& pool = *tdbb->getDefaultPool();

// Get first the array descriptor, then the array

SLONG stuff[IAD_LEN(16) / 4];
Ods::InternalArrayDesc* array_desc = (Ods::InternalArrayDesc*) stuff;
blb* blob = blb::get_array(tdbb, tdbb->getRequest()->req_transaction, (bid*)value->dsc_address,
array_desc);
blb* blob = blb::get_array(tdbb, tdbb->getRequest()->req_transaction,
(bid*) value->dsc_address, array_desc);

fb_assert(array_desc->iad_total_length >= 0); // check before upcasting to size_t
UCHAR* data = FB_NEW_POOL(*getDefaultMemoryPool()) UCHAR[static_cast<size_t>(array_desc->iad_total_length)];
AutoPtr<UCHAR, ArrayDelete> data(FB_NEW_POOL(pool) UCHAR[array_desc->iad_total_length]);
blob->BLB_get_data(tdbb, data, array_desc->iad_total_length);
const USHORT dimensions = array_desc->iad_dimensions;

Expand All @@ -1012,39 +1013,29 @@ static SLONG get_scalar_array(const Parameter* arg,
dsc from = array_desc->iad_rpt[0].iad_desc;

if (to.dsc_dtype != from.dsc_dtype ||
to.dsc_scale != from.dsc_scale || to.dsc_length != from.dsc_length)
to.dsc_scale != from.dsc_scale ||
to.dsc_length != from.dsc_length)
{
SLONG n = array_desc->iad_count;
UCHAR* const temp = FB_NEW_POOL(*getDefaultMemoryPool()) UCHAR[static_cast<size_t>(to.dsc_length * n)];
ULONG n = array_desc->iad_count;
AutoPtr<UCHAR, ArrayDelete> temp(FB_NEW_POOL(pool) UCHAR[to.dsc_length * n]);
to.dsc_address = temp;
from.dsc_address = data;

// This loop may call ERR_post indirectly.
try
{
for (; n; --n, to.dsc_address += to.dsc_length,
from.dsc_address += array_desc->iad_element_length)
{
MOV_move(tdbb, &from, &to);
}
}
catch (const Exception&)
for (; n; --n, to.dsc_address += to.dsc_length,
from.dsc_address += array_desc->iad_element_length)
{
delete[] data;
delete[] temp;
throw;
MOV_move(tdbb, &from, &to);
}

delete[] data;
data = temp;
data = temp.release();
}

// Fill out the scalar array descriptor

stack.push(data);
scalar_desc->sad_desc = arg->prm_desc;
scalar_desc->sad_desc.dsc_address = data;
scalar_desc->sad_dimensions = dimensions;
stack.push(data.release());

This comment has been minimized.

Sign in to view

Copy link
@dmitry-lipetsk

dmitry-lipetsk Nov 5, 2023

Contributor

Can stack.push throw OOM-exception?

This comment has been minimized.

Sign in to view

Copy link
@AlexPeshkoff

AlexPeshkoff Nov 6, 2023

Member

Definitely yes

This comment has been minimized.

Sign in to view

Copy link
@dmitry-lipetsk

dmitry-lipetsk Nov 9, 2023

Contributor 
stack.push(nullptr);
const_cast<UCHAR&>(stack.back())=data.release();
fb_assert(!data);
fb_assert(stack.back());

?

This comment has been minimized.

Sign in to view

Copy link
@dyemanov

dyemanov Nov 9, 2023

Author Member

Given the Stack object has 16 static (stack-backed) elements and legacy UDFs are limited by 15 arguments, I'd say OOM is nearly impossible.

This comment has been minimized.

Sign in to view

Copy link
@dyemanov

dyemanov Nov 9, 2023

Author Member

Oh, I seem being wrong here. The empty stack doesn't seem to have any static part, 16 elements is its allocation granularity. Sorry, will fix the code (although it looks quite paranoid ;-).

This comment has been minimized.

Sign in to view

Copy link
@hvlad

hvlad Nov 9, 2023

Member

Time to add static part ?

This comment has been minimized.

Sign in to view

Copy link
@dmitry-lipetsk

dmitry-lipetsk Nov 9, 2023

Contributor

I found that container.push( autoPtr.release()) is used in other parts of code, too.

This comment has been minimized.

Sign in to view

Copy link
@dyemanov

dyemanov Nov 9, 2023

Author Member

Time to add static part ?

In this case I'd just replace Stack with Vector<UCHAR*, MAX_UDF_ARGUMENTS> as it's just a simple collection of pointers to be deleted after the call.

This comment has been minimized.

Sign in to view

Copy link
@dmitry-lipetsk

dmitry-lipetsk Nov 9, 2023

Contributor

Just add stack.reserve(MAX_UDF_ARGUMENTS) after declaration :)

This comment has been minimized.

Sign in to view

Copy link
@dyemanov

dyemanov Nov 9, 2023

Author Member

Just add stack.reserve(MAX_UDF_ARGUMENTS) after declaration :)

The simplest fix, but it costs one extra allocation cycle ;-)

This comment has been minimized.

Sign in to view

Copy link
@hvlad

hvlad Nov 9, 2023

Member

Time to add static part ?

In this case I'd just replace Stack with Vector<UCHAR*, MAX_UDF_ARGUMENTS> as it's just a simple collection of pointers to be deleted after the call.

Sure. I mean general case, not this one.

const Ods::InternalArrayDesc::iad_repeat* tail1 = array_desc->iad_rpt;
scalar_array_desc::sad_repeat* tail2 = scalar_desc->sad_rpt;
Expand All @@ -1055,7 +1046,8 @@ static SLONG get_scalar_array(const Parameter* arg,
tail2->sad_lower = tail1->iad_lower;
}

return static_cast<SLONG>(sizeof(scalar_array_desc) + (dimensions - 1u) * sizeof(scalar_array_desc::sad_repeat));
return static_cast<ULONG>(sizeof(scalar_array_desc) +
(dimensions - 1u) * sizeof(scalar_array_desc::sad_repeat));
}


Expand Down

0 comments on commit 749bcc0

Please sign in to comment.