Skip to content

GH-48985: [GLib][Ruby] Fix GC problems in node options and expressions #48989

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

Merged
kou merged 1 commit into from
Jan 27, 2026
+442 −22
Merged

GH-48985: [GLib][Ruby] Fix GC problems in node options and expressions #48989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
196 changes: 184 additions & 12 deletions c_glib/arrow-glib/compute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,9 +1274,71 @@ garrow_source_node_options_new_table(GArrowTable *table)
return options;
}

G_DEFINE_TYPE(GArrowFilterNodeOptions,
garrow_filter_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)
enum {
PROP_EXPRESSION = 1,
};

struct GArrowFilterNodeOptionsPrivate
{
GArrowExpression *expression;
};

G_DEFINE_TYPE_WITH_PRIVATE(GArrowFilterNodeOptions,
garrow_filter_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)

#define GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object) \
static_cast<GArrowFilterNodeOptionsPrivate *>( \
garrow_filter_node_options_get_instance_private(GARROW_FILTER_NODE_OPTIONS(object)))

static void
garrow_filter_node_options_dispose(GObject *object)
{
auto priv = GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object);

if (priv->expression) {
g_object_unref(priv->expression);
priv->expression = nullptr;
}

G_OBJECT_CLASS(garrow_filter_node_options_parent_class)->dispose(object);
}
Comment on lines +1295 to +1305
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

GArrowFilterNodeOptionsPrivate::expression is used in garrow_filter_node_options_dispose() and garrow_filter_node_options_get_property() but is never initialized in garrow_filter_node_options_init(). To avoid undefined behavior when options are constructed without the "expression" property (or if construction fails early), initialize expression to NULL in the init function before it is accessed here.

Copilot uses AI. Check for mistakes.

static void
garrow_filter_node_options_set_property(GObject *object,
guint prop_id,
const GValue *value,
GParamSpec *pspec)
{
auto priv = GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object);

switch (prop_id) {
case PROP_EXPRESSION:
priv->expression = GARROW_EXPRESSION(g_value_dup_object(value));
break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
break;
}
}

static void
garrow_filter_node_options_get_property(GObject *object,
guint prop_id,
GValue *value,
GParamSpec *pspec)
{
auto priv = GARROW_FILTER_NODE_OPTIONS_GET_PRIVATE(object);

switch (prop_id) {
case PROP_EXPRESSION:
g_value_set_object(value, priv->expression);
break;
default:
G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
break;
}
}

static void
garrow_filter_node_options_init(GArrowFilterNodeOptions *object)
Expand All @@ -1286,6 +1348,28 @@ garrow_filter_node_options_init(GArrowFilterNodeOptions *object)
static void
garrow_filter_node_options_class_init(GArrowFilterNodeOptionsClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);

gobject_class->dispose = garrow_filter_node_options_dispose;
gobject_class->set_property = garrow_filter_node_options_set_property;
gobject_class->get_property = garrow_filter_node_options_get_property;

GParamSpec *spec;

/**
* GArrowFilterNodeOptions:expression:
*
* The expression of this filter.
*
* Since: 24.0.0
*/
spec = g_param_spec_object(
"expression",
"Expression",
"The expression of this filter",
GARROW_TYPE_EXPRESSION,
static_cast<GParamFlags>(G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));
g_object_class_install_property(gobject_class, PROP_EXPRESSION, spec);
}

/**
Expand All @@ -1301,14 +1385,39 @@ garrow_filter_node_options_new(GArrowExpression *expression)
{
auto arrow_expression = garrow_expression_get_raw(expression);
auto arrow_options = new arrow::acero::FilterNodeOptions(*arrow_expression);
auto options =
g_object_new(GARROW_TYPE_FILTER_NODE_OPTIONS, "options", arrow_options, NULL);
auto options = g_object_new(GARROW_TYPE_FILTER_NODE_OPTIONS,
"options",
arrow_options,
"expression",
expression,
nullptr);
return GARROW_FILTER_NODE_OPTIONS(options);
}

G_DEFINE_TYPE(GArrowProjectNodeOptions,
garrow_project_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)
struct GArrowProjectNodeOptionsPrivate
{
GList *expressions;
};

G_DEFINE_TYPE_WITH_PRIVATE(GArrowProjectNodeOptions,
garrow_project_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)

#define GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(object) \
static_cast<GArrowProjectNodeOptionsPrivate *>( \
garrow_project_node_options_get_instance_private( \
GARROW_PROJECT_NODE_OPTIONS(object)))

static void
garrow_project_node_options_dispose(GObject *object)
{
auto priv = GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(object);

g_list_free_full(priv->expressions, g_object_unref);
priv->expressions = nullptr;

G_OBJECT_CLASS(garrow_project_node_options_parent_class)->dispose(object);
Comment on lines +1412 to +1419
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

GArrowProjectNodeOptionsPrivate::expressions is freed in garrow_project_node_options_dispose() but is never initialized in garrow_project_node_options_init(), so g_list_free_full() may receive an indeterminate pointer if the field was not set. Please initialize expressions to NULL in the init function to make dispose safe for partially-constructed or default instances.

Copilot uses AI. Check for mistakes.
}

static void
garrow_project_node_options_init(GArrowProjectNodeOptions *object)
Expand All @@ -1318,6 +1427,9 @@ garrow_project_node_options_init(GArrowProjectNodeOptions *object)
static void
garrow_project_node_options_class_init(GArrowProjectNodeOptionsClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);

gobject_class->dispose = garrow_project_node_options_dispose;
}

/**
Expand Down Expand Up @@ -1354,9 +1466,28 @@ garrow_project_node_options_new(GList *expressions, gchar **names, gsize n_names
new arrow::acero::ProjectNodeOptions(arrow_expressions, arrow_names);
auto options =
g_object_new(GARROW_TYPE_PROJECT_NODE_OPTIONS, "options", arrow_options, NULL);
auto priv = GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(options);
priv->expressions =
g_list_copy_deep(expressions, reinterpret_cast<GCopyFunc>(g_object_ref), nullptr);
return GARROW_PROJECT_NODE_OPTIONS(options);
}

/**
* garrow_project_node_options_get_expressions:
* @options: A #GArrowProjectNodeOptions.
*
* Returns: (transfer none) (element-type GArrowExpression): Expressions
* of the @options.
*
* Since: 24.0.0
*/
GList *
garrow_project_node_options_get_expressions(GArrowProjectNodeOptions *options)
{
auto priv = GARROW_PROJECT_NODE_OPTIONS_GET_PRIVATE(options);
return priv->expressions;
}

typedef struct GArrowAggregationPrivate_
{
gchar *function;
Expand Down Expand Up @@ -1558,9 +1689,28 @@ garrow_aggregation_new(const gchar *function,
NULL));
}

G_DEFINE_TYPE(GArrowAggregateNodeOptions,
garrow_aggregate_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)
struct GArrowAggregateNodeOptionsPrivate
{
GList *aggregations;
};

G_DEFINE_TYPE_WITH_PRIVATE(GArrowAggregateNodeOptions,
garrow_aggregate_node_options,
GARROW_TYPE_EXECUTE_NODE_OPTIONS)

#define GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(object) \
static_cast<GArrowAggregateNodeOptionsPrivate *>( \
garrow_aggregate_node_options_get_instance_private( \
GARROW_AGGREGATE_NODE_OPTIONS(object)))

static void
garrow_aggregate_node_options_dispose(GObject *object)
{
auto priv = GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(object);
g_list_free_full(priv->aggregations, g_object_unref);
priv->aggregations = nullptr;
G_OBJECT_CLASS(garrow_aggregate_node_options_parent_class)->dispose(object);
}
Comment on lines +1707 to +1713
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

GArrowAggregateNodeOptionsPrivate::aggregations is freed in garrow_aggregate_node_options_dispose() but never initialized in garrow_aggregate_node_options_init(), which can cause g_list_free_full() to operate on garbage if the field isn't explicitly set. As with other GObject classes in this file, this member should be initialized to NULL in the init function before being used here.

Copilot uses AI. Check for mistakes.

static void
garrow_aggregate_node_options_init(GArrowAggregateNodeOptions *object)
Expand All @@ -1570,6 +1720,9 @@ garrow_aggregate_node_options_init(GArrowAggregateNodeOptions *object)
static void
garrow_aggregate_node_options_class_init(GArrowAggregateNodeOptionsClass *klass)
{
auto gobject_class = G_OBJECT_CLASS(klass);

gobject_class->dispose = garrow_aggregate_node_options_dispose;
}

/**
Expand Down Expand Up @@ -1623,10 +1776,29 @@ garrow_aggregate_node_options_new(GList *aggregations,
auto arrow_options = new arrow::acero::AggregateNodeOptions(std::move(arrow_aggregates),
std::move(arrow_keys));
auto options =
g_object_new(GARROW_TYPE_AGGREGATE_NODE_OPTIONS, "options", arrow_options, NULL);
g_object_new(GARROW_TYPE_AGGREGATE_NODE_OPTIONS, "options", arrow_options, nullptr);
auto priv = GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(options);
priv->aggregations =
g_list_copy_deep(aggregations, reinterpret_cast<GCopyFunc>(g_object_ref), nullptr);
return GARROW_AGGREGATE_NODE_OPTIONS(options);
}

/**
* garrow_aggregate_node_options_get_aggregations:
* @options: A #GArrowAggregateNodeOptions.
*
* Returns: (transfer none) (element-type GArrowAggregation): Aggregations
* of the @options.
*
* Since: 24.0.0
*/
GList *
garrow_aggregate_node_options_get_aggregations(GArrowAggregateNodeOptions *options)
{
auto priv = GARROW_AGGREGATE_NODE_OPTIONS_GET_PRIVATE(options);
return priv->aggregations;
}

typedef struct GArrowSinkNodeOptionsPrivate_
{
arrow::AsyncGenerator<std::optional<arrow::compute::ExecBatch>> generator;
Expand Down
8 changes: 8 additions & 0 deletions c_glib/arrow-glib/compute.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ GARROW_AVAILABLE_IN_11_0
GArrowProjectNodeOptions *
garrow_project_node_options_new(GList *expressions, gchar **names, gsize n_names);

GARROW_AVAILABLE_IN_24_0
GList *
garrow_project_node_options_get_expressions(GArrowProjectNodeOptions *options);

#define GARROW_TYPE_AGGREGATION (garrow_aggregation_get_type())
GARROW_AVAILABLE_IN_6_0
G_DECLARE_DERIVABLE_TYPE(
Expand Down Expand Up @@ -218,6 +222,10 @@ garrow_aggregate_node_options_new(GList *aggregations,
gsize n_keys,
GError **error);

GARROW_AVAILABLE_IN_24_0
GList *
garrow_aggregate_node_options_get_aggregations(GArrowAggregateNodeOptions *options);

#define GARROW_TYPE_SINK_NODE_OPTIONS (garrow_sink_node_options_get_type())
GARROW_AVAILABLE_IN_6_0
G_DECLARE_DERIVABLE_TYPE(GArrowSinkNodeOptions,
Expand Down
Loading
Loading