parser BUGFIX move extension instance parents on array reallocation

[mardgez13]

Every struct lysp_ext_instance keeps a parent pointer to the statement it belongs to. That statement is often an item of a sized array (a typedef, enum, must, revision, import, ...), a member of such an item (&enum->flags for status, &tpdf->dflt for default), or the base of a separate array with parent_stmt_index (iffeatures, bases). When LYA_ADD_ITEM() grows the array and realloc() moves it, every such parent points into the freed memory.

• parser_yin.c never updated these parents after adding an item, so they went stale whenever the array moved, and were later read through them (e.g. by lysp_ext_instance_path()).
• parser_yang.c updated statement array items with a loop after each add, but reset every parent to the item itself, losing the member offsets of substatement parents (status, default, ...); parents into separate arrays such as iffeatures were not updated at all.
• lysp_inject_submodule() grows the main module's includes without updating them either.

The fix adds two macros: LYSP_ARRAY_EXT_PARENTS_UPDATE(ARRAY, EXTS, STMT[, MEMBER]) sets the parent of the extension instances of the substatement STMT of every item of an array to the item itself or to its MEMBER (such as flags), and LYSP_EXT_PARENTS_UPDATE(EXTS, STMT, PARENT) does it for one array of extension instances, for the separate arrays (if-feature, base, default, unique). They are used at every place an array with extension instance parents grows, in both parsers and in lysp_inject_submodule(), each listing the substatements whose parents point into the array, and replace the per-site loops in parser_yang.c.

The second commit fixes another reference into a growing array in the YIN parser. yin_parse_fracdigits(), yin_pasrse_reqinstance() and yin_parse_value_pos() stored their parent's extension instance array (type->exts, enm->exts) in ctx->ext_inst while the parent type, enum or bit could still add more extension instances to it. A later extension instance of the parent could move the array, and lysp_resolve_ext_instance_records() then read the freed memory. The parent statements already store their array once they are parsed, as in the YANG parser, so these three calls are removed; every remaining call stores the array of the statement that was just parsed.

The third commit makes the YIN parser give the extension instances of an if-feature, unique or the default of a leaf-list, refine or deviate the array as their parent and the index of the item, as the YANG parser does. It created them always with the index 0 (and for a default with the item as the parent), so the printers and lysp_ext_instance_path() took them all for instances of the first item.

Tests:

• test_ext_parent_array_realloc in tests/utests/schema/test_schema.c parses the same module in YANG and YIN with three typedefs (with default), enums (with status), musts, if-features and leaf-list defaults, each with extension instances, and checks every parent points to its own statement, member or array, with the index of its if-feature or default.
• tests/utests/basic/test_plugins.c loads the extension plugin test module in YIN too and adds musts to it; the parse callback now checks that the instance is the one stored in its parent, for enums, union types, musts, typedefs and refines in both formats.
• test_yin_parse_type_substmt_exts in tests/utests/schema/test_yin.c parses fraction-digits, require-instance, value and position with an extension instance, each followed by more extension instances of its parent, and checks all of them are kept in order.

The full test suite passes (ASan/UBSan build).

[michalvasko]

Thanks, but will require some changes. I appreciate the tests and those will be merged but the new code is too complex and unreadable. Ideally, remove the old_mem value completely and I think you can simply define two macros instead of one macro and two functions.

[mardgez13]

Thanks for the review. I replaced the two functions and the macro with two macros: LYSP_EXT_PARENTS_MOVE() moves the parents in one array of extension instances and LYSP_ARRAY_EXT_PARENTS_MOVE() does it for the extension instances of every item of an array. I also rebased the branch on the current devel.

I kept old_mem, though. Some parents point to a member of an item rather than to the item itself, for example status to &enm->flags, default to &tpdf->dflt, min-elements to &rf->min or revision-date to imp->rev. The old address is the only way to tell which member a parent points to, so that it still points to it after the move. Setting such parents to the item, as the YANG parser did before, makes lysp_ext_instance_path() and the plugins read the item as flags or as a qname, and the new tests check two of these cases. If you see a way to do this without old_mem, I am happy to change it.

[michalvasko]

What I meant is that you do not have to check whether the array start has actually changed but always overwrite it. It is not like every statement has hundreds of extensions, makes no difference but not needing the old_mem variable.

[mardgez13]

The macros do not check whether the array moved, they always overwrite. The if selects the parents that point into this array, and old_mem tells where in it they point, because the exts of an item also hold the instances of its substatements:

enum a {
  if-feature f { ex:e; }      // parent: enm->iffeatures, a separate array
  status current { ex:e; }    // parent: &enm->flags
  ex:e;                       // parent: enm
}

All three instances are in enm->exts. Overwriting every parent with &enums[u] turns the first two into the enum itself, so lysp_ext_instance_path() and the plugins read the enum as an array of qnames or as flags. The same goes for default (&tpdf->dflt), revision-date (imp->rev), min-elements and max-elements (&rf->min, &rf->max), and base in identities and union types (the separate bases array). The new tests check two of these (check_ext_parents() expects &en->flags and &tpdf->dflt), so they fail with an unconditional overwrite.

To overwrite without the old address, the macro would have to know which member each substatement points to, so a switch on parent_stmt for every statement type. The old address gives the same result with one subtraction, which is why I think keeping it is the simpler option. If you would rather not have the variable in the functions, I can move it into the macros so that they add the item themselves, replacing LYA_ADD_ITEM() at these places.

[michalvasko]

Okay, then you can pass the substatement (member) into the macro. With some macro magic a single macro may even support both variants, an array item member or the item itself.

[mardgez13]

Done, old_mem is gone. LYSP_ARRAY_EXT_PARENTS_UPDATE(ARRAY, EXTS, STMT[, MEMBER]) sets the parent of the extension instances of the substatement STMT of every item to the item itself, or to its MEMBER when given (a single macro, GETMACRO4() picks the variant as in LY_CHECK_RET()). LYSP_EXT_PARENTS_UPDATE(EXTS, STMT, PARENT) does the same for one array of extension instances and is used for the separate arrays (if-feature, base, default, unique). Each place lists the substatements whose parents point into the array, for example for enums and bits:

LYSP_ARRAY_EXT_PARENTS_UPDATE(*enums, exts, enum_kw);
LYSP_ARRAY_EXT_PARENTS_UPDATE(*enums, exts, LY_STMT_VALUE);
LYSP_ARRAY_EXT_PARENTS_UPDATE(*enums, exts, LY_STMT_POSITION);
LYSP_ARRAY_EXT_PARENTS_UPDATE(*enums, exts, LY_STMT_STATUS, flags);

The YIN lists also have description and reference, because the YIN parser sets the statement itself as their parent, while the YANG parser leaves it NULL.

Always setting the array as the parent needed one more fix in the YIN parser, in its own commit: it created the extension instances of an if-feature, unique or the default of a leaf-list, refine or deviate always with the index 0 (and for a default with the item as the parent), so the printers took them all for instances of the first item. yin_parse_qname() now gives them the array and the item index as the YANG parser does, and test_ext_parent_array_realloc checks the indexes and leaf-list defaults too.

I checked the parent of every extension instance of a module with several items in each of these arrays and extension instances in all their substatements, in both formats: in YANG they are the same as with the previous version, in YIN they differ only in those indexes and default parents. The full test suite passes (ASan/UBSan).