From 73e81b3c5fc6cf83b299c9cecde9b2e6f5211c15 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Mon, 5 Apr 2021 16:46:49 -0400 Subject: [PATCH 1/4] wip: allow Property table (0x17) EnCLog entries in pass1 --- src/mono/mono/metadata/metadata-update.c | 36 +++++++++++++++++++----- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/mono/mono/metadata/metadata-update.c b/src/mono/mono/metadata/metadata-update.c index 67aeec0cba968..e9d66c58d0518 100644 --- a/src/mono/mono/metadata/metadata-update.c +++ b/src/mono/mono/metadata/metadata-update.c @@ -814,11 +814,25 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer int token_table = mono_metadata_token_table (log_token); int token_index = mono_metadata_token_index (log_token); - if (token_table == MONO_TABLE_ASSEMBLYREF) { + switch (token_table) { + case MONO_TABLE_ASSEMBLYREF: /* okay, supported */ - } else if (token_table == MONO_TABLE_METHOD) { + break; + case MONO_TABLE_METHOD: /* handled above */ - } else { + break; + case MONO_TABLE_PROPERTY: + if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { + /* modifying a property */ + break; + } else { + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support adding new properties.", i, log_token); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support adding new properties. token=0x%08x", log_token); + unsupported_edits = TRUE; + continue; + } + default: + /* FIXME: this bounds check is wrong for cumulative updates - need to look at the DeltaInfo:count.prev_gen_rows */ if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token); mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token); @@ -884,7 +898,8 @@ apply_enclog_pass2 (MonoImage *image_base, uint32_t generation, MonoImage *image break; } - if (token_table == MONO_TABLE_ASSEMBLYREF) { + switch (token_table) { + case MONO_TABLE_ASSEMBLYREF: { g_assert (token_index > table_info_get_rows (&image_base->tables [token_table])); if (assemblyref_updated) @@ -916,7 +931,9 @@ apply_enclog_pass2 (MonoImage *image_base, uint32_t generation, MonoImage *image mono_image_unlock (image_base); g_free (old_array); - } else if (token_table == MONO_TABLE_METHOD) { + break; + } + case MONO_TABLE_METHOD: { if (token_index > table_info_get_rows (&image_base->tables [token_table])) { g_error ("EnC: new method added, should be caught by pass1"); } @@ -935,17 +952,22 @@ apply_enclog_pass2 (MonoImage *image_base, uint32_t generation, MonoImage *image /* rva points probably into image_base IL stream. can this ever happen? */ g_print ("TODO: this case is still a bit contrived. token=0x%08x with rva=0x%04x\n", log_token, rva); } - } else if (token_table == MONO_TABLE_TYPEDEF) { + break; + } + case MONO_TABLE_TYPEDEF: { /* TODO: throw? */ /* TODO: happens changing the class (adding field or method). we ignore it, but dragons are here */ /* existing entries are supposed to be patched */ g_assert (token_index <= table_info_get_rows (&image_base->tables [token_table])); - } else { + break; + } + default: { g_assert (token_index > table_info_get_rows (&image_base->tables [token_table])); if (mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE)) g_print ("todo: do something about this table index: 0x%02x\n", token_table); } + } } return TRUE; } From c1925c981a9bec7eb142fe43297990bae807c818 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Tue, 6 Apr 2021 16:30:15 -0400 Subject: [PATCH 2/4] [mbr] Allow MONO_TABLE_PROPERTY updates in pass2 Note that modifications of getter or setter bodies add new rows to the MethodSemantics table that point to an existing method def (with an updated IL) and to an existing row in the property table. So there is nothing to do. --- src/mono/mono/metadata/metadata-update.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/mono/mono/metadata/metadata-update.c b/src/mono/mono/metadata/metadata-update.c index e9d66c58d0518..9992d93dc069d 100644 --- a/src/mono/mono/metadata/metadata-update.c +++ b/src/mono/mono/metadata/metadata-update.c @@ -962,6 +962,13 @@ apply_enclog_pass2 (MonoImage *image_base, uint32_t generation, MonoImage *image g_assert (token_index <= table_info_get_rows (&image_base->tables [token_table])); break; } + case MONO_TABLE_PROPERTY: { + /* allow updates to existing properties. */ + /* FIXME: use DeltaInfo:prev_gen_rows instead of image_base */ + g_assert (token_index <= table_info_get_rows (&image_base->tables [token_table])); + /* assuming that property attributes and type haven't changed. */ + break; + } default: { g_assert (token_index > table_info_get_rows (&image_base->tables [token_table])); if (mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE)) From e2eb9476d5ae0d89d466e9485fbfc8421791ddd2 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Apr 2021 13:07:24 -0400 Subject: [PATCH 3/4] [mbr] Add pass1 check that new MethodSemantics are pointing at existing methods. We only support modifications of getters/setters, not adding a new getter or setter where there wasn't one previously. (Adding new methods in general fails right now) --- src/mono/mono/metadata/metadata-update.c | 43 ++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/metadata-update.c b/src/mono/mono/metadata/metadata-update.c index 9992d93dc069d..a9df3ffdac8fe 100644 --- a/src/mono/mono/metadata/metadata-update.c +++ b/src/mono/mono/metadata/metadata-update.c @@ -831,6 +831,39 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer unsupported_edits = TRUE; continue; } + case MONO_TABLE_METHODSEMANTICS: { + if (token_index > table_info_get_rows (&image_base->tables [token_table])) { + /* new rows are fine, as long as they point at existing methods */ + guint32 sema_cols [MONO_METHOD_SEMA_SIZE]; + int mapped_token = mono_image_relative_delta_index (image_dmeta, mono_metadata_make_token (token_table, token_index)); + g_assert (mapped_token != -1); + mono_metadata_decode_row (&image_dmeta->tables [MONO_TABLE_METHODSEMANTICS], mapped_token - 1, sema_cols, MONO_METHOD_SEMA_SIZE); + + switch (sema_cols [MONO_METHOD_SEMA_SEMANTICS]) { + case METHOD_SEMANTIC_GETTER: + case METHOD_SEMANTIC_SETTER: { + int prop_method_index = sema_cols [MONO_METHOD_SEMA_METHOD]; + /* ok, if it's pointing to an existing getter/setter */ + if (prop_method_index < table_info_get_rows (&image_base->tables [MONO_TABLE_METHOD])) + break; + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x adding new getter/setter method 0x%08x to a property is not supported", i, log_token, prop_method_index); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support adding a new getter or setter to a property, token=0x%08x", log_token); + unsupported_edits = TRUE; + continue; + } + default: + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x adding new non-getter/setter property or event methods is not supported.", i, log_token); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support adding new non-getter/setter property or event methods. token=0x%08x", log_token); + unsupported_edits = TRUE; + continue; + } + } else { + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token); + unsupported_edits = TRUE; + continue; + } + } default: /* FIXME: this bounds check is wrong for cumulative updates - need to look at the DeltaInfo:count.prev_gen_rows */ if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { @@ -1045,6 +1078,14 @@ mono_image_load_enc_delta (MonoImage *image_base, gconstpointer dmeta_bytes, uin return; } + /* Process EnCMap and compute number of added/modified rows from this + * delta. This enables computing row indexes relative to the delta. + * We use it in pass1 to bail out early if the EnCLog has unsupported + * edits. + */ + delta_info_init (image_dmeta, image_base); + + if (!apply_enclog_pass1 (image_base, image_dmeta, dil_bytes, dil_length, error)) { mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "Error on sanity-checking delta image to base=%s, due to: %s", basename, mono_error_get_message (error)); mono_metadata_update_cancel (generation); @@ -1055,8 +1096,6 @@ mono_image_load_enc_delta (MonoImage *image_base, gconstpointer dmeta_bytes, uin if (table_info_get_rows (table_enclog)) table_to_image_add (image_base); - delta_info_init (image_dmeta, image_base); - mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "base guid: %s", image_base->guid); mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "dmeta guid: %s", image_dmeta->guid); From e4da7d28a31ae473ba445a855c36616966ddf88a Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Wed, 14 Apr 2021 12:42:51 -0400 Subject: [PATCH 4/4] address feedback --- src/mono/mono/metadata/metadata-update.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/mono/mono/metadata/metadata-update.c b/src/mono/mono/metadata/metadata-update.c index a9df3ffdac8fe..a9916f5261683 100644 --- a/src/mono/mono/metadata/metadata-update.c +++ b/src/mono/mono/metadata/metadata-update.c @@ -821,16 +821,16 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer case MONO_TABLE_METHOD: /* handled above */ break; - case MONO_TABLE_PROPERTY: - if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { - /* modifying a property */ + case MONO_TABLE_PROPERTY: { + /* modifying a property, ok */ + if (token_index <= table_info_get_rows (&image_base->tables [token_table])) break; - } else { - mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support adding new properties.", i, log_token); - mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support adding new properties. token=0x%08x", log_token); - unsupported_edits = TRUE; - continue; - } + /* adding a property */ + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support adding new properties.", i, log_token); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support adding new properties. token=0x%08x", log_token); + unsupported_edits = TRUE; + continue; + } case MONO_TABLE_METHODSEMANTICS: { if (token_index > table_info_get_rows (&image_base->tables [token_table])) { /* new rows are fine, as long as they point at existing methods */