Skip to content

follow-up: pdo_sqlite: Use the new FCC API #14059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

follow-up: pdo_sqlite: Use the new FCC API #14059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9b0c64
Use the new FCC API
SakiTakamachi Apr 28, 2024
82c2bac
Address comments
SakiTakamachi May 1, 2024
0ad7272
added tests
SakiTakamachi May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions ext/pdo_sqlite/pdo_sqlite.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,33 +335,25 @@ PHP_METHOD(PdoSqlite, openBlob)
static int php_sqlite_collation_callback(void *context, int string1_len, const void *string1,
int string2_len, const void *string2)
{
int ret;
int ret = 0;
zval zargs[2];
zval retval;
struct pdo_sqlite_collation *collation = (struct pdo_sqlite_collation*) context;

collation->fc.fci.size = sizeof(collation->fc.fci);
ZVAL_COPY_VALUE(&collation->fc.fci.function_name, &collation->callback);
collation->fc.fci.object = NULL;
collation->fc.fci.retval = &retval;

// Prepare the arguments.
ZVAL_STRINGL(&zargs[0], (char *) string1, string1_len);
ZVAL_STRINGL(&zargs[1], (char *) string2, string2_len);
collation->fc.fci.param_count = 2;
collation->fc.fci.params = zargs;

if ((ret = zend_call_function(&collation->fc.fci, &collation->fc.fcc)) == FAILURE) {
php_error_docref(NULL, E_WARNING, "An error occurred while invoking the callback");
} else if (!Z_ISUNDEF(retval)) {
zend_call_known_fcc(&collation->callback, &retval, /* argc */ 2, zargs, /* named_params */ NULL);

if (!Z_ISUNDEF(retval)) {
if (Z_TYPE(retval) != IS_LONG) {
zend_string *func_name = get_active_function_or_method_name();
zend_type_error("%s(): Return value of the callback must be of type int, %s returned",
ZSTR_VAL(func_name), zend_zval_value_name(&retval));
zend_string_release(func_name);
return FAILURE;
Copy link
Member

Choose a reason for hiding this comment

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

While you're at it, can you change this return FAILURE, as I would like them to limit their use to functions that actually have a return type of zend_result.

Or this can be a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will also follow up on this.

}
ret = 0;
if (Z_LVAL(retval) > 0) {
ret = 1;
} else if (Z_LVAL(retval) < 0) {
Expand Down
13 changes: 4 additions & 9 deletions ext/pdo_sqlite/php_pdo_sqlite_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,28 +26,23 @@ typedef struct {
char *errmsg;
} pdo_sqlite_error_info;

struct pdo_sqlite_fci {
zend_fcall_info fci;
zend_fcall_info_cache fcc;
};

struct pdo_sqlite_func {
struct pdo_sqlite_func *next;

zval func, step, fini;
int argc;
Copy link
Member

Choose a reason for hiding this comment

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

Aside: is this argc field even used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, here is.

php-src/ext/pdo_sqlite/sqlite_driver.c

Line 108 in 6303d1f

func->argc,

const char *funcname;

/* accelerated callback references */
struct pdo_sqlite_fci afunc, astep, afini;
zend_fcall_info_cache func;
zend_fcall_info_cache step;
zend_fcall_info_cache fini;
};

struct pdo_sqlite_collation {
struct pdo_sqlite_collation *next;

const char *name;
zval callback;
struct pdo_sqlite_fci fc;
zend_fcall_info_cache callback;
};

typedef struct {
Expand Down
115 changes: 57 additions & 58 deletions ext/pdo_sqlite/sqlite_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ static void pdo_sqlite_cleanup_callbacks(pdo_sqlite_db_handle *H)
}

efree((char*)func->funcname);
if (!Z_ISUNDEF(func->func)) {
zval_ptr_dtor(&func->func);
if (ZEND_FCC_INITIALIZED(func->func)) {
zend_fcc_dtor(&func->func);
}
if (!Z_ISUNDEF(func->step)) {
zval_ptr_dtor(&func->step);
if (ZEND_FCC_INITIALIZED(func->step)) {
zend_fcc_dtor(&func->step);
}
if (!Z_ISUNDEF(func->fini)) {
zval_ptr_dtor(&func->fini);
if (ZEND_FCC_INITIALIZED(func->fini)) {
zend_fcc_dtor(&func->fini);
}
efree(func);
}
Expand All @@ -139,8 +139,8 @@ static void pdo_sqlite_cleanup_callbacks(pdo_sqlite_db_handle *H)
}

efree((char*)collation->name);
if (!Z_ISUNDEF(collation->callback)) {
zval_ptr_dtor(&collation->callback);
if (ZEND_FCC_INITIALIZED(collation->callback)) {
zend_fcc_dtor(&collation->callback);
}
efree(collation);
}
Expand Down Expand Up @@ -309,12 +309,12 @@ typedef struct {
zend_long row;
} aggregate_context;

static int do_callback(struct pdo_sqlite_fci *fc, zval *cb, int argc, sqlite3_value **argv, sqlite3_context *context, int is_agg)
static int do_callback(zend_fcall_info_cache *fcc, int argc, sqlite3_value **argv, sqlite3_context *context, int is_agg)
{
zval *zargs = NULL;
zval retval;
int i;
int ret;
int ret = SUCCESS;
Copy link
Member

Choose a reason for hiding this comment

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

Either this function should return a zend_result or use integers.

It also looks like we don't even use the return value when calling this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. However, there are probably some other similar ones, so I will respond with a follow-up PR.

int fake_argc;
aggregate_context *agg_context = NULL;

Expand All @@ -324,14 +324,7 @@ static int do_callback(struct pdo_sqlite_fci *fc, zval *cb, int argc, sqlite3_va

fake_argc = argc + is_agg;

fc->fci.size = sizeof(fc->fci);
ZVAL_COPY_VALUE(&fc->fci.function_name, cb);
fc->fci.object = NULL;
fc->fci.retval = &retval;
fc->fci.param_count = fake_argc;

/* build up the params */

if (fake_argc) {
zargs = safe_emalloc(fake_argc, sizeof(zval), 0);
}
Expand Down Expand Up @@ -372,11 +365,7 @@ static int do_callback(struct pdo_sqlite_fci *fc, zval *cb, int argc, sqlite3_va
}
}

fc->fci.params = zargs;

if ((ret = zend_call_function(&fc->fci, &fc->fcc)) == FAILURE) {
php_error_docref(NULL, E_WARNING, "An error occurred while invoking the callback");
}
zend_call_known_fcc(fcc, &retval, fake_argc, zargs, /* named_params */ NULL);

/* clean up the params */
if (zargs) {
Expand Down Expand Up @@ -445,41 +434,33 @@ static void php_sqlite3_func_step_callback(sqlite3_context *context, int argc, s
{
struct pdo_sqlite_func *func = (struct pdo_sqlite_func*)sqlite3_user_data(context);

do_callback(&func->astep, &func->step, argc, argv, context, 1);
do_callback(&func->step, argc, argv, context, 1);
}

static void php_sqlite3_func_final_callback(sqlite3_context *context)
{
struct pdo_sqlite_func *func = (struct pdo_sqlite_func*)sqlite3_user_data(context);

do_callback(&func->afini, &func->fini, 0, NULL, context, 1);
do_callback(&func->fini, 0, NULL, context, 1);
}

static int php_sqlite3_collation_callback(void *context, int string1_len, const void *string1, int string2_len, const void *string2)
{
int ret;
int ret = 0;
zval zargs[2];
zval retval;
struct pdo_sqlite_collation *collation = (struct pdo_sqlite_collation*) context;

collation->fc.fci.size = sizeof(collation->fc.fci);
ZVAL_COPY_VALUE(&collation->fc.fci.function_name, &collation->callback);
collation->fc.fci.object = NULL;
collation->fc.fci.retval = &retval;

/* Prepare the arguments. */
ZVAL_STRINGL(&zargs[0], (char *) string1, string1_len);
ZVAL_STRINGL(&zargs[1], (char *) string2, string2_len);
collation->fc.fci.param_count = 2;
collation->fc.fci.params = zargs;

if ((ret = zend_call_function(&collation->fc.fci, &collation->fc.fcc)) == FAILURE) {
php_error_docref(NULL, E_WARNING, "An error occurred while invoking the callback");
} else if (!Z_ISUNDEF(retval)) {
zend_call_known_fcc(&collation->callback, &retval, /* argc */ 2, zargs, /* named_params */ NULL);

if (!Z_ISUNDEF(retval)) {
if (Z_TYPE(retval) != IS_LONG) {
convert_to_long(&retval);
}
ret = 0;
if (Z_LVAL(retval) > 0) {
ret = 1;
} else if (Z_LVAL(retval) < 0) {
Expand All @@ -498,14 +479,14 @@ static void php_sqlite3_func_callback(sqlite3_context *context, int argc, sqlite
{
struct pdo_sqlite_func *func = (struct pdo_sqlite_func*)sqlite3_user_data(context);

do_callback(&func->afunc, &func->func, argc, argv, context, 0);
do_callback(&func->func, argc, argv, context, 0);
}

void pdo_sqlite_create_function_internal(INTERNAL_FUNCTION_PARAMETERS)
{
struct pdo_sqlite_func *func;
zend_fcall_info fci;
zend_fcall_info_cache fcc;
zend_fcall_info fci = empty_fcall_info;
zend_fcall_info_cache fcc = empty_fcall_info_cache;
char *func_name;
size_t func_name_len;
zend_long argc = -1;
Expand All @@ -516,11 +497,11 @@ void pdo_sqlite_create_function_internal(INTERNAL_FUNCTION_PARAMETERS)

ZEND_PARSE_PARAMETERS_START(2, 4)
Z_PARAM_STRING(func_name, func_name_len)
Z_PARAM_FUNC(fci, fcc)
Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(fci, fcc)
Copy link
Member

Choose a reason for hiding this comment

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

This would leak if you have a trampoline and one of the follow-up arguments fails the ZPP check I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, indeed. This will leak.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it 82c2bac

Z_PARAM_OPTIONAL
Z_PARAM_LONG(argc)
Z_PARAM_LONG(flags)
ZEND_PARSE_PARAMETERS_END();
ZEND_PARSE_PARAMETERS_END_EX(goto error;);

dbh = Z_PDO_DBH_P(ZEND_THIS);
PDO_CONSTRUCT_CHECK;
Expand All @@ -533,7 +514,7 @@ void pdo_sqlite_create_function_internal(INTERNAL_FUNCTION_PARAMETERS)
if (ret == SQLITE_OK) {
func->funcname = estrdup(func_name);

ZVAL_COPY(&func->func, &fci.function_name);
zend_fcc_dup(&func->func, &fcc);

func->argc = argc;

Expand All @@ -544,6 +525,9 @@ void pdo_sqlite_create_function_internal(INTERNAL_FUNCTION_PARAMETERS)
}

efree(func);

error:
zend_release_fcall_info_cache(&fcc);
RETURN_FALSE;
}

Expand All @@ -558,8 +542,10 @@ PHP_METHOD(PDO_SQLite_Ext, sqliteCreateFunction)
void pdo_sqlite_create_aggregate_internal(INTERNAL_FUNCTION_PARAMETERS)
{
struct pdo_sqlite_func *func;
zend_fcall_info step_fci, fini_fci;
zend_fcall_info_cache step_fcc, fini_fcc;
zend_fcall_info step_fci = empty_fcall_info;
zend_fcall_info fini_fci = empty_fcall_info;
zend_fcall_info_cache step_fcc = empty_fcall_info_cache;
zend_fcall_info_cache fini_fcc = empty_fcall_info_cache;
char *func_name;
size_t func_name_len;
zend_long argc = -1;
Expand All @@ -569,11 +555,11 @@ void pdo_sqlite_create_aggregate_internal(INTERNAL_FUNCTION_PARAMETERS)

ZEND_PARSE_PARAMETERS_START(3, 4)
Z_PARAM_STRING(func_name, func_name_len)
Z_PARAM_FUNC(step_fci, step_fcc)
Z_PARAM_FUNC(fini_fci, fini_fcc)
Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(step_fci, step_fcc)
Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(fini_fci, fini_fcc)
Z_PARAM_OPTIONAL
Z_PARAM_LONG(argc)
ZEND_PARSE_PARAMETERS_END();
ZEND_PARSE_PARAMETERS_END_EX(goto error;);

dbh = Z_PDO_DBH_P(ZEND_THIS);
PDO_CONSTRUCT_CHECK;
Expand All @@ -587,9 +573,8 @@ void pdo_sqlite_create_aggregate_internal(INTERNAL_FUNCTION_PARAMETERS)
if (ret == SQLITE_OK) {
func->funcname = estrdup(func_name);

ZVAL_COPY(&func->step, &step_fci.function_name);

ZVAL_COPY(&func->fini, &fini_fci.function_name);
zend_fcc_dup(&func->step, &step_fcc);
zend_fcc_dup(&func->fini, &fini_fcc);

func->argc = argc;

Expand All @@ -600,6 +585,10 @@ void pdo_sqlite_create_aggregate_internal(INTERNAL_FUNCTION_PARAMETERS)
}

efree(func);

error:
zend_release_fcall_info_cache(&step_fcc);
zend_release_fcall_info_cache(&fini_fcc);
RETURN_FALSE;
}

Expand Down Expand Up @@ -631,8 +620,8 @@ PHP_METHOD(PDO_SQLite_Ext, sqliteCreateAggregate)
void pdo_sqlite_create_collation_internal(INTERNAL_FUNCTION_PARAMETERS, pdo_sqlite_create_collation_callback callback)
{
struct pdo_sqlite_collation *collation;
zend_fcall_info fci;
zend_fcall_info_cache fcc;
zend_fcall_info fci = empty_fcall_info;
zend_fcall_info_cache fcc = empty_fcall_info_cache;
char *collation_name;
size_t collation_name_len;
pdo_dbh_t *dbh;
Expand All @@ -641,7 +630,7 @@ void pdo_sqlite_create_collation_internal(INTERNAL_FUNCTION_PARAMETERS, pdo_sqli

ZEND_PARSE_PARAMETERS_START(2, 2)
Z_PARAM_STRING(collation_name, collation_name_len)
Z_PARAM_FUNC(fci, fcc)
Z_PARAM_FUNC_NO_TRAMPOLINE_FREE(fci, fcc)
ZEND_PARSE_PARAMETERS_END();

dbh = Z_PDO_DBH_P(ZEND_THIS);
Expand All @@ -655,14 +644,16 @@ void pdo_sqlite_create_collation_internal(INTERNAL_FUNCTION_PARAMETERS, pdo_sqli
if (ret == SQLITE_OK) {
collation->name = estrdup(collation_name);

ZVAL_COPY(&collation->callback, &fci.function_name);
zend_fcc_dup(&collation->callback, &fcc);

collation->next = H->collations;
H->collations = collation;

RETURN_TRUE;
}

zend_release_fcall_info_cache(&fcc);

if (UNEXPECTED(EG(exception))) {
RETURN_THROWS();
}
Expand Down Expand Up @@ -706,15 +697,23 @@ static void pdo_sqlite_get_gc(pdo_dbh_t *dbh, zend_get_gc_buffer *gc_buffer)

struct pdo_sqlite_func *func = H->funcs;
while (func) {
zend_get_gc_buffer_add_zval(gc_buffer, &func->func);
zend_get_gc_buffer_add_zval(gc_buffer, &func->step);
zend_get_gc_buffer_add_zval(gc_buffer, &func->fini);
if (ZEND_FCC_INITIALIZED(func->func)) {
zend_get_gc_buffer_add_fcc(gc_buffer, &func->func);
}
if (ZEND_FCC_INITIALIZED(func->step)) {
zend_get_gc_buffer_add_fcc(gc_buffer, &func->step);
}
if (ZEND_FCC_INITIALIZED(func->fini)) {
zend_get_gc_buffer_add_fcc(gc_buffer, &func->fini);
}
func = func->next;
}

struct pdo_sqlite_collation *collation = H->collations;
while (collation) {
zend_get_gc_buffer_add_zval(gc_buffer, &collation->callback);
if (ZEND_FCC_INITIALIZED(collation->callback)) {
zend_get_gc_buffer_add_fcc(gc_buffer, &collation->callback);
}
collation = collation->next;
}
}
Expand Down
Loading
Loading