Skip to content

Commit

Permalink
[#22342] YSQL: load_relcache_init_file should clear yb_table_properties
Browse files Browse the repository at this point in the history
Summary:
1. Related data structures:

```
typedef struct YbTablePropertiesData* YbTableProperties;

typedef struct RelationData
{
    ...
    YbTableProperties yb_table_properties; /* NULL if not loaded */
    ...
} RelationData;
```

Note that `yb_table_properties` is a pointer.

2. In `write_relcache_init_file`

```
        /* first write the relcache entry proper */
        write_item(rel, sizeof(RelationData), fp);
```
This is going to write the contents of RelationData as raw bytes to the rel cache init file fp.

3. In `load_relcache_init_file`, we load the raw bytes back into RelationData:

```
        /* first read the relation descriptor length */
        nread = fread(&len, 1, sizeof(len), fp);
        if (nread != sizeof(len))
        {
            if (nread == 0)
                break;          /* end of file */
            goto read_failed;
        }

        /* safety check for incompatible relcache layout */
        if (len != sizeof(RelationData))
            goto read_failed;

        /* allocate another relcache header */
        if (num_rels >= max_rels)
        {
            max_rels *= 2;
            rels = (Relation *) repalloc(rels, max_rels * sizeof(Relation));
        }

        rel = rels[num_rels++] = (Relation) palloc(len);

        /* then, read the Relation structure */
        if (fread(rel, 1, len, fp) != len)
            goto read_failed;
```

4. The bug here is that `yb_table_properties` is a pointer, it is a private memory
address in the PG backend process that invoked `write_relcache_init_file`, and
then it will be interpreted within the PG backend process that invoked
`load_relcache_init_file`. It is not valid to do that because it can point to some
random bytes meant for other things. In the worst case the same raw pointer may
point to an address that is not even mmap'ed into the PG backend process that
loads the data and can lead to SEGV if accessed.

This diff fixes the bug by adding to `load_relcache_init_file`:
```
        /* YB properties will be loaded lazily */
        rel->yb_table_properties = NULL;
```
We are already clearing yb_table_properties in `AllocateRelationDesc`.

Note:

This appears to be a recent change. In the past when `write_relcache_init_file` is invoked, we have not set `yb_table_properties` but after commit `07db0b57cee0aee372a6820638fd07cb20c499a9` we are because
in `ybcSetupScanPlan` we now call `YbGetTableProperties`:

```
static void
ybcSetupScanPlan(bool xs_want_itup, YbScanDesc ybScan, YbScanPlan scan_plan)
{
    Relation relation = ybScan->relation;
    Relation index = ybScan->index;
    YbTableProperties yb_table_prop_relation = YbGetTableProperties(relation);
```
Jira: DB-11248

Test Plan: jenkins

Reviewers: kfranz, fizaa

Reviewed By: fizaa

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D34931
  • Loading branch information
myang2021 committed May 10, 2024
1 parent dae8cd5 commit 0486cac
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions src/postgres/src/backend/utils/cache/relcache.c
Original file line number Diff line number Diff line change
Expand Up @@ -7893,6 +7893,9 @@ load_relcache_init_file(bool shared)
rel->rd_amcache = NULL;
MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));

/* YB properties will be loaded lazily */
rel->yb_table_properties = NULL;

/*
* Recompute lock and physical addressing info. This is needed in
* case the pg_internal.init file was copied from some other database
Expand Down

0 comments on commit 0486cac

Please sign in to comment.