Critical: fix inverted logic here

[sfc-gh-dachristensen]

strcmp(str, "") returns 0 (false) when str is empty, meaning the check is inverted: it returns NULL when parsing succeeds and continues when parsing fails. This allows non-numeric strings to pass through as array indices, leading to type confusion and potentially incorrect memory access.

The strcmp logic handles most cases correctly (non-numeric strings return NULL, valid integers pass through). However, the empty string "" is accepted as a valid array index of 0: [10, 20, 30] #> '[""]' returns 10 instead of NULL. This occurs because strtol("") sets lindex=0 and str="", so strcmp("", "") returns 0, bypassing the error check.

[Copilot]

Pull request overview

This PR aims to fix agtype path array-index parsing so that an empty string ("") is not accepted as a valid array index (currently treated as 0), which can allow invalid/non-numeric path elements to be used as array subscripts.

Changes:

• Updates the strtol() parse validation check from if (strcmp(str, "")) to if (strcmp(str, "") != 0) within get_agtype_path_all().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/backend/utils/adt/agtype_ops.c:2114

• strtol() overflow is not checked via errno. On platforms where sizeof(long) == sizeof(int) (e.g., 32-bit builds), an out-of-range value can return LONG_MAX/LONG_MIN (equal to INT_MAX/INT_MIN) and pass the subsequent lindex > INT_MAX || lindex < INT_MIN guard. Consider following the pattern used in src/backend/utils/adt/graphid.c:75-78 by setting errno = 0 before strtol() and returning NULL when errno != 0.

                char* str = NULL;
                lindex = strtol(cur_key->val.string.val, &str, 10);

                if (strcmp(str, "") != 0)
                {
                    PG_RETURN_NULL();
                }
            }
            else
            {
                PG_RETURN_NULL();
            }

            if (lindex > INT_MAX || lindex < INT_MIN)
            {
                PG_RETURN_NULL();
            }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new condition strcmp(str, "") != 0 is logically equivalent to the previous if (strcmp(str, "")) and still does not reject the empty-string case described in the PR. When the input is "" strtol() leaves str pointing at the start of the string, so strcmp(str, "") is 0 and this check is bypassed, accepting index 0. To reject empty strings (and generally ensure at least one digit was consumed), check str == cur_key->val.string.val (endptr == start) in addition to requiring endptr to be at the string terminator.

[Copilot]

This change is fixing path/index parsing for the #> / #>> operators, and the repo already has regression coverage for these operators (e.g., regress/sql/jsonb_operators.sql section "Agtype path extraction operators"). Please add a regression test case that asserts ... #> '[""]' returns NULL (and ideally that a large out-of-range numeric string is rejected), to prevent this from regressing again.

[jrgemignani]

@sfc-gh-dachristensen Please fix the above Copilot issues.

[jrgemignani]

See copilot review

[gregfelice]

@sfc-gh-dachristensen — I think this change is a no-op, and the title overstates it. Recommending we close unless I'm missing something.

The diff is:

-                if (strcmp(str, ""))
+                if (strcmp(str, "") != 0)

These are identical in C. strcmp() returns 0 when the strings are equal and non-zero otherwise, and if (x) already tests x != 0. So both forms mean "if str is not empty" and compile to the same thing. There is no inverted logic here — the existing code is already correct.

The semantics of the surrounding block are also right as-is: strtol() sets str to the first unparsed character, so strcmp(str, "") being true means there was trailing garbage after the number (e.g. "1abc"), and the code correctly bails with PG_RETURN_NULL(). An empty leftover means the whole key parsed cleanly as an integer.

If the actual concern is the one Copilot describes — that an empty key "" is accepted and silently treated as index 0, because strtol("") returns 0 with str pointing at the terminator so the guard passes — then that is a real issue, but this diff does not fix it. That would need an explicit check that the key is non-empty (and ideally an errno/ERANGE check around strtol) before using lindex.

Happy to look at a revised PR that addresses the empty-key case with a regress test. As it stands I'd close this one.

[sfc-gh-dachristensen]

@gregfelice on re-review, agreed this PR as currently written is nonsense—will close and review to see what the original report I was responding to was referencing. This code block clearly intends to return NULL if we have any extra chars in the parsed string, which it does do properly. Sorry for the noise.