fix: prevent segfault when package has subproc with param-initialized local var (#1005) by rophy · Pull Request #1123 · IvorySQL/IvorySQL

rophy · 2025-12-07T10:11:02Z

Summary

Fix segmentation fault when executing any function in a PL/iSQL package that contains a private procedure with a local variable
initialized from a parameter
Add regression tests covering the fix

Root Cause

When compiling nested subprocedures (private procedures/functions within a package or standalone function), the subproc's datums
(including parameters and local variables) were incorrectly included in the parent function's initvarnos array by
plisql_add_initdatums().

When the parent function was called, it attempted to initialize these subproc datums, but since the subproc hadn't been invoked yet, the
datum memory wasn't properly allocated - causing a segfault.

Fix

Added plisql_datum_belongs_to_subproc() helper function to check if a datum index falls within any registered subproc's datum range.
Modified plisql_add_initdatums() to skip datums that belong to subprocs.

This ensures subproc variables are only initialized when the subproc itself is called, not when the parent function is entered.

Test Plan

All 17 PL/iSQL regression tests pass
oracle-check-world passes
Added 5 new test cases in plisql_nested_subproc.sql:
1. Package with private proc having param-initialized local var (never called)
2. Same package but actually calling the private proc
3. Standalone function with nested procedure
4. Nested function that is never called
5. Multiple nested subprocs with param-initialized local vars
Verified Oracle compatibility behavior matches

Fixes #1005

Summary by CodeRabbit

Bug Fixes
- Fixed issue Segfault when package procedure initializes local variable from parameter #1005: outer-block initialization no longer erroneously initializes local variables belonging to nested subprocedures, aligning initialization with subprocedure semantics.
Tests
- Added comprehensive tests covering nested subprocedures and local variables initialized from parameters, including package-based and standalone scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

… local var Skip subproc datums in plisql_add_initdatums() to prevent parent block from initializing nested function/procedure variables. This fixes crash when subproc local var default references its own parameter. Fixes: IvorySQL#1005

coderabbitai · 2025-12-07T10:11:31Z

Walkthrough

Prevents parent-block initialization of datums that belong to nested subprocedures by adding a helper to detect subproc-owned datum indices and skipping those datums during init-datums collection. Adds SQL tests exercising nested subprocedures and parameter-initialized local variables.

Changes

Cohort / File(s)	Summary
PL/iSQL Compilation Logic `src/pl/plisql/src/pl_comp.c`	Adds static helper `plisql_datum_belongs_to_subproc(int)` to detect datum indices within subprocedure datum ranges; updates `plisql_add_initdatums()` to skip subproc-owned datums in both the outer datum loop and the inner varnos aggregation.
Test Coverage `src/pl/plisql/src/sql/plisql_nested_subproc.sql`	Adds multiple tests (package `test_pkg_1005`, `test_func_1005`, `test_func_1005_nocall`, `test_func_1005_multi`, etc.) exercising nested subprocedures and locals initialized from parameters to validate the fix for issue `#1005`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Review plisql_datum_belongs_to_subproc() loop bounds and conditions over plisql_nsubprocFuncs.
Verify both call sites in plisql_add_initdatums() correctly exclude subproc datums and maintain varnos ordering/semantics.

Possibly related PRs

plisql code for function and procedure #921 — Related changes in subproc datum-range fields and headers that this patch depends on.
package phase II #945 — Additional nested-subprocedure handling changes touching the same datum/subproc logic.
package phase II #944 — Broad subprocedure/OUT-parameter support that defines datum ranges used by this fix.

Suggested reviewers

jiaoshuntian
NotHimmel
OreoYang

Poem

🐰 In tangled scopes a bug did creep,
Subproc datums woke when they should sleep.
I hop in, check each index with care,
Skip the ones that live in nested lair,
Now packages run smooth — a carrot cheer! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the primary fix: preventing a segfault in packages with subprocedures that have parameter-initialized local variables, and references the linked issue #1005.
Linked Issues check	✅ Passed	The PR fully addresses issue #1005's primary objective: fixing the segmentation fault by implementing proper datum initialization logic for subprocedures and adding comprehensive regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the segfault issue: the helper function and modification to plisql_add_initdatums() address the root cause, and test cases validate the fix across multiple scenarios.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d23c62a and 6f1a164.

⛔ Files ignored due to path filters (1)

src/pl/plisql/src/expected/plisql_nested_subproc.out is excluded by !**/*.out

📒 Files selected for processing (1)

src/pl/plisql/src/sql/plisql_nested_subproc.sql (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

GitHub Check: oracle_regression (ubuntu-latest)
GitHub Check: oracle_pg_regression (ubuntu-latest)
GitHub Check: meson_build (ubuntu-latest)
GitHub Check: contrib_regression
GitHub Check: build (ubuntu-latest)
GitHub Check: pg_regression (ubuntu-latest)

🔇 Additional comments (1)

src/pl/plisql/src/sql/plisql_nested_subproc.sql (1)

3774-3892: Well-designed regression tests that directly target the fix for issue #1005.

The five new test cases comprehensively validate the segfault prevention when nested subprocedures initialize local variables from parameters:

Test 1–2 (lines 3778–3817): Package-level private procedure with param-initialized local; first variant calls the parent without invoking the private procedure (core fix scenario), second variant verifies the procedure works when actually invoked.

Test 3 (lines 3822–3839): Standalone function with called nested procedure; validates the fix applies beyond packages.

Test 4 (lines 3842–3857): Nested function declared but never invoked; critical edge case that directly exercises the datum-skipping logic the fix introduces.

Test 5 (lines 3861–3890): Multiple nested subprocs with mixed call/no-call scenarios; appropriately notes issue v_sum := v_sum + nested_func() ignores prior changes from nested procedure #1124 blocking func1 invocation.

Each test is self-documenting with expected outputs and clear comments. The progression from simple to complex and package to standalone coverage is appropriate for a regression suite. All SQL syntax is correct.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/pl/plisql/src/sql/plisql_nested_subproc.sql (1)

3860-3888: Minor: Test 5 could be more comprehensive.

The func1 nested function is defined with a param-initialized local variable (line 3876) but is never invoked or used in the return calculation. While this doesn't detract from validating the crash fix, including a call to func1 or incorporating its return value into the aggregation (e.g., v_sum := v_sum + func1(10)) would strengthen confidence in the fix across all nested subprogram types. The inline comment on line 3883 suggests this is a known limitation; if intentional, no action is needed.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7261f1c and d23c62a.

⛔ Files ignored due to path filters (2)

src/pl/plisql/src/expected/plisql_nested_subproc.out is excluded by !**/*.out
src/pl/plisql/src/expected/plisql_type_rowtype.out is excluded by !**/*.out

📒 Files selected for processing (2)

src/pl/plisql/src/pl_comp.c (3 hunks)
src/pl/plisql/src/sql/plisql_nested_subproc.sql (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: rophy
Repo: IvorySQL/IvorySQL PR: 998
File: contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c:82-146
Timestamp: 2025-12-03T06:50:27.755Z
Learning: In contrib/ivorysql_ora/src/builtin_packages/dbms_output/dbms_output.c, the behavior where ENABLE() clears the existing buffer (rather than preserving it like Oracle does) is an intentional known difference tracked in issue #26 at https://github.com/rophy/IvorySQL/issues/26 (medium impact, open).

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

GitHub Check: oracle_regression (ubuntu-latest)
GitHub Check: build (ubuntu-latest)
GitHub Check: contrib_regression
GitHub Check: pg_regression (ubuntu-latest)
GitHub Check: meson_build (ubuntu-latest)
GitHub Check: oracle_pg_regression (ubuntu-latest)

🔇 Additional comments (3)

src/pl/plisql/src/pl_comp.c (2)

3285-3320: ✓ Correct helper function to identify subproc datums

The new plisql_datum_belongs_to_subproc() function correctly identifies whether a datum index belongs to any subprocedure by iterating through all subprocs and checking if the datum falls within each subproc's range [lastassignvardno, lastoutvardno). The guards on subproc->function == NULL && subproc->lastoutvardno == 0 properly exclude unfinalized or forward-declared subprocs.

3348-3359: ✓ Consistent application of subproc datum skip logic

Both the datum-counting loop and the varnos-building loop in plisql_add_initdatums() consistently skip subproc datums. This ensures that subproc parameters and local variables are never added to the parent block's initialization list, preventing the segfault when the parent entry tries to initialize memory that belongs to the subproc's execution context.

Also applies to: 3382-3384

src/pl/plisql/src/sql/plisql_nested_subproc.sql (1)

3773-3890: Excellent comprehensive test coverage for issue #1005.

The five test cases effectively validate the fix by covering the key scenarios:

Package with private procedure (param-initialized local; called and not called)

Standalone function with nested procedure (called)

Standalone function with nested function (never called)

Multiple nested subprocedures and functions

The tests exercise both the crash-prevention path (uncalled subprocs) and the functional path (called subprocs) as intended.

Add 5 test cases covering the segfault in PL/iSQL packages with private procedures having local variables initialized from parameters: 1. Package with private proc that has param-initialized local var 2. Call the private proc to verify it works when invoked 3. Standalone function with nested procedure 4. Nested function with param-initialized var that is never called 5. Multiple nested subprocs with param-initialized local vars These tests verify the fix prevents the crash when: - A private procedure has a local var initialized from its parameter - The parent function is called without invoking the private proc

yasir-hussain-shah

@rophy
Fix is working good. Regression tests are well written which covers related cases.
Just a question out of my curiosity, the changes in the file
src/pl/plisql/src/expected/plisql_type_rowtype.out
seems unrelated to this fix. To verify, I reverted changes in this file and noted that: make oracle-check still passes, however, make oracle-check-world causes a regression in above file. That's strange.

Regards,

Yasir Hussain Shah
Data Bene

rophy · 2025-12-10T20:34:19Z

@rophy Fix is working good. Regression tests are well written which covers related cases. Just a question out of my curiosity, the changes in the file src/pl/plisql/src/expected/plisql_type_rowtype.out seems unrelated to this fix. To verify, I reverted changes in this file and noted that: make oracle-check still passes, however, make oracle-check-world causes a regression in above file. That's strange.

Regards,

Yasir Hussain Shah Data Bene

The change to plisql_type_rowtype.out is
intentional and a direct result of the fix.

Before the fix: When fun4 was called, the parent function attempted to initialize inf1's local variables at entry time. The error context showed fun4 because the error occurred in the parent's initialization phase.

After the fix: Nested function (inf1) variables are only initialized when inf1 is actually called. Now the error context correctly shows:

PL/iSQL function inf1 line 2 during statement block local variable initialization - the actual location of the error
PL/iSQL function fun4(pg_catalog.int4) line 7 at RETURN - the call stack showing
where inf1 was invoked

This is more correct behavior - the error now points to the nested function inf1 where the problematic variable initialization occurs, rather than incorrectly attributing it to the parent function fun4.

This also matches Oracle's behavior where nested function variables are initialized at call time, not at parent function entry time.

bigplaice · 2026-01-08T09:34:42Z

This fix looks very good and solid. Thanks for your contribution again!

coderabbitai bot reviewed Dec 7, 2025

View reviewed changes

rophy force-pushed the fix/1005 branch from d23c62a to 6f1a164 Compare December 7, 2025 10:21

yasir-hussain-shah reviewed Dec 10, 2025

View reviewed changes

gaoxueyu merged commit 538691f into IvorySQL:master Jan 9, 2026
6 checks passed

rophy deleted the fix/1005 branch January 9, 2026 23:32

coderabbitai bot mentioned this pull request Jan 27, 2026

fix: prevent segfault when package has subproc with param-initialized… #1173

Merged

gaoxueyu added this to IvorySQL 5.3 Mar 12, 2026

github-project-automation bot moved this to Done in IvorySQL 5.3 Mar 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent segfault when package has subproc with param-initialized local var (#1005)#1123

fix: prevent segfault when package has subproc with param-initialized local var (#1005)#1123
gaoxueyu merged 2 commits intoIvorySQL:masterfrom
rophy:fix/1005

rophy commented Dec 7, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 7, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

yasir-hussain-shah left a comment

Uh oh!

rophy commented Dec 10, 2025

Uh oh!

bigplaice commented Jan 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rophy commented Dec 7, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause

Fix

Test Plan

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

yasir-hussain-shah left a comment

Choose a reason for hiding this comment

Uh oh!

rophy commented Dec 10, 2025

Uh oh!

bigplaice commented Jan 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rophy commented Dec 7, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 7, 2025 •

edited

Loading