Skip to content

Commit

Permalink
[mono] CodeQL fix set #1 (#99637)
Browse files Browse the repository at this point in the history
CodeQL flagged various places where we're dereferencing pointers that could be NULL, this PR systematically cleans some of them up via g_assert.
* g_assert result of g_build_path calls
* Allocation failure handling
* mono_class_inflate_generic_class_checked can return NULL
  • Loading branch information
kg authored Mar 14, 2024
1 parent f529d5d commit 172ecdf
Show file tree
Hide file tree
Showing 14 changed files with 110 additions and 71 deletions.
1 change: 1 addition & 0 deletions src/mono/mono/eglib/gfile-posix.c
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ g_file_open_tmp (const gchar *tmpl, gchar **name_used, GError **gerror)
}

t = g_build_filename (g_get_tmp_dir (), tmpl, (const char*)NULL);
g_assert (t);

#ifdef HOST_WASI
g_critical ("g_file_open_tmp is not implemented for WASI");
Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/metadata/appdomain.c
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ try_load_from (MonoAssembly **assembly,

*assembly = NULL;
fullpath = g_build_filename (path1, path2, path3, path4, (const char*)NULL);
g_assert (fullpath);

found = g_file_test (fullpath, G_FILE_TEST_IS_REGULAR);

Expand Down
2 changes: 2 additions & 0 deletions src/mono/mono/metadata/assembly.c
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ load_in_path (const char *basename, const char** search_path, const MonoAssembly

for (i = 0; search_path [i]; ++i) {
fullpath = g_build_filename (search_path [i], basename, (const char*)NULL);
g_assert (fullpath);
result = mono_assembly_request_open (fullpath, req, status);
g_free (fullpath);
if (result)
Expand Down Expand Up @@ -1407,6 +1408,7 @@ absolute_dir (const gchar *filename)

cwd = g_get_current_dir ();
mixed = g_build_filename (cwd, filename, (const char*)NULL);
g_assert (mixed);
parts = g_strsplit (mixed, G_DIR_SEPARATOR_S, 0);
g_free (mixed);
g_free (cwd);
Expand Down
7 changes: 6 additions & 1 deletion src/mono/mono/metadata/class.c
Original file line number Diff line number Diff line change
Expand Up @@ -6785,10 +6785,13 @@ mono_method_get_base_method (MonoMethod *method, gboolean definition, MonoError
if (mono_class_is_open_constructed_type (m_class_get_byval_arg (parent))) {
parent = mono_class_inflate_generic_class_checked (parent, generic_inst, error);
return_val_if_nok (error, NULL);
g_assert (parent);
}

if (mono_class_is_ginst (parent)) {
parent_inst = mono_class_get_context (parent);
parent = mono_class_get_generic_class (parent)->container_class;
g_assert (parent);
}

mono_class_setup_vtable (parent);
Expand All @@ -6808,6 +6811,7 @@ mono_method_get_base_method (MonoMethod *method, gboolean definition, MonoError
if (mono_class_is_open_constructed_type (m_class_get_byval_arg (klass))) {
klass = mono_class_inflate_generic_class_checked (klass, generic_inst, error);
return_val_if_nok (error, NULL);
g_assert (klass);

generic_inst = NULL;
}
Expand All @@ -6821,6 +6825,7 @@ mono_method_get_base_method (MonoMethod *method, gboolean definition, MonoError
if (generic_inst) {
klass = mono_class_inflate_generic_class_checked (klass, generic_inst, error);
return_val_if_nok (error, NULL);
g_assert (klass);
generic_inst = NULL;
}

Expand Down Expand Up @@ -6909,7 +6914,7 @@ mono_class_has_default_constructor (MonoClass *klass, gboolean public_only)
* \param klass class in which the failure was detected
* \param fmt \c printf -style error message string.
*
* Sets a deferred failure in the class and prints a warning message.
* Sets a deferred failure in the class and prints a warning message.
* The deferred failure allows the runtime to attempt setting up the class layout at runtime.
*
* LOCKING: Acquires the loader lock.
Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/metadata/icall.c
Original file line number Diff line number Diff line change
Expand Up @@ -4564,6 +4564,7 @@ ves_icall_System_Reflection_RuntimeAssembly_GetInfo (MonoQCallAssemblyHandle ass
else
absolute = g_build_filename (assembly->basedir, filename, (const char*)NULL);

g_assert (absolute);
mono_icall_make_platform_path (absolute);

const gchar *prepend = mono_icall_get_file_path_prefix (absolute);
Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/metadata/image.c
Original file line number Diff line number Diff line change
Expand Up @@ -2511,6 +2511,7 @@ mono_image_load_file_for_image_checked (MonoImage *image, uint32_t fileidx, Mono
fname = mono_metadata_string_heap (image, fname_id);
base_dir = g_path_get_dirname (image->name);
name = g_build_filename (base_dir, fname, (const char*)NULL);
g_assert (name);
res = mono_image_open (name, NULL);
if (!res)
goto done;
Expand Down
4 changes: 3 additions & 1 deletion src/mono/mono/metadata/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -4243,6 +4243,8 @@ prepare_run_main (MonoMethod *method, int argc, char *argv[])
basename,
(const char*)NULL);

g_assert (fullpath);

utf8_fullpath = utf8_from_external (fullpath);
if(utf8_fullpath == NULL) {
/* Printing the arg text will cause glib to
Expand Down Expand Up @@ -5355,7 +5357,7 @@ MonoObjectHandle
mono_object_new_handle (MonoClass *klass, MonoError *error)
{
MONO_REQ_GC_UNSAFE_MODE;

if (MONO_CLASS_IS_IMPORT(klass)) {
mono_error_set_not_supported (error, "Built-in COM interop is not supported on Mono.");
return MONO_HANDLE_NEW (MonoObject, NULL);
Expand Down
4 changes: 3 additions & 1 deletion src/mono/mono/metadata/sgen-tarjan-bridge.c
Original file line number Diff line number Diff line change
Expand Up @@ -819,8 +819,10 @@ create_scc (ScanData *data)
g_error ("Invalid state when building SCC %d", other->state);
}

if (other->is_bridge)
if (other->is_bridge) {
g_assert (color_data);
dyn_array_ptr_add (&color_data->bridges, other->obj);
}

// Maybe we should make sure we are not adding duplicates here. It is not really a problem
// since we will get rid of duplicates before submitting the SCCs to the client in gather_xrefs
Expand Down
19 changes: 17 additions & 2 deletions src/mono/mono/mini/aot-compiler.c
Original file line number Diff line number Diff line change
Expand Up @@ -5508,10 +5508,10 @@ MONO_RESTORE_WARNING
if (decoded_args->named_args_info [j].field && !strcmp (decoded_args->named_args_info [j].field->name, "EntryPoint")) {
named = (const char *)decoded_args->named_args[j]->value.primitive;
slen = mono_metadata_decode_value (named, &named);

int prefix_len = (int)strlen (acfg->user_symbol_prefix);
g_assert (prefix_len < 2);

export_name = (char *)g_malloc (prefix_len + slen + 1);
if (prefix_len == 1)
export_name[0] = *acfg->user_symbol_prefix;
Expand Down Expand Up @@ -5851,12 +5851,14 @@ add_generic_class_with_depth (MonoAotCompile *acfg, MonoClass *klass, int depth,

icomparable_inst = mono_class_inflate_generic_class_checked (icomparable, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (icomparable_inst);

if (mono_class_is_assignable_from_internal (icomparable_inst, tclass)) {
MonoClass *gcomparer_inst;
gcomparer = mono_class_load_from_name (mono_defaults.corlib, "System.Collections.Generic", "GenericComparer`1");
gcomparer_inst = mono_class_inflate_generic_class_checked (gcomparer, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (gcomparer_inst);

add_generic_class (acfg, gcomparer_inst, FALSE, "Comparer<T>");
}
Expand All @@ -5878,13 +5880,15 @@ add_generic_class_with_depth (MonoAotCompile *acfg, MonoClass *klass, int depth,

iface_inst = mono_class_inflate_generic_class_checked (iface, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (iface_inst);

if (mono_class_is_assignable_from_internal (iface_inst, tclass)) {
MonoClass *gcomparer_inst;

gcomparer = mono_class_load_from_name (mono_defaults.corlib, "System.Collections.Generic", "GenericEqualityComparer`1");
gcomparer_inst = mono_class_inflate_generic_class_checked (gcomparer, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (gcomparer_inst);
add_generic_class (acfg, gcomparer_inst, FALSE, "EqualityComparer<T>");
}
}
Expand All @@ -5906,6 +5910,7 @@ add_generic_class_with_depth (MonoAotCompile *acfg, MonoClass *klass, int depth,
enum_comparer = mono_class_load_from_name (mono_defaults.corlib, "System.Collections.Generic", "EnumEqualityComparer`1");
enum_comparer_inst = mono_class_inflate_generic_class_checked (enum_comparer, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (enum_comparer_inst);
add_generic_class (acfg, enum_comparer_inst, FALSE, "EqualityComparer<T>");
}
}
Expand All @@ -5927,6 +5932,7 @@ add_generic_class_with_depth (MonoAotCompile *acfg, MonoClass *klass, int depth,
comparer = mono_class_load_from_name (mono_defaults.corlib, "System.Collections.Generic", "ObjectComparer`1");
comparer_inst = mono_class_inflate_generic_class_checked (comparer, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (comparer_inst);
add_generic_class (acfg, comparer_inst, FALSE, "Comparer<T>");
}
}
Expand All @@ -5950,6 +5956,7 @@ add_instances_of (MonoAotCompile *acfg, MonoClass *klass, MonoType **insts, int
ctx.class_inst = mono_metadata_get_generic_inst (1, args);
generic_inst = mono_class_inflate_generic_class_checked (klass, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (generic_inst);
add_generic_class (acfg, generic_inst, force, "");
}
}
Expand Down Expand Up @@ -11566,6 +11573,9 @@ emit_exception_info (MonoAotCompile *acfg)
char *aot_file = g_strdup_printf("%s%s", image_basename, SEQ_POINT_AOT_EXT);
char *aot_file_path = g_build_filename (dir, aot_file, (const char*)NULL);

g_assert (dir);
g_assert (aot_file_path);

if (g_ensure_directory_exists (aot_file_path) == FALSE) {
fprintf (stderr, "AOT : failed to create msym directory: %s\n", aot_file_path);
exit (1);
Expand Down Expand Up @@ -15345,6 +15355,8 @@ set_paths (MonoAotCompile *acfg)
}

acfg->tmpbasename = g_build_filename (temp_path, "temp", (const char*)NULL);
g_assert (acfg->tmpbasename);

acfg->asm_fname = g_strdup_printf ("%s.s", acfg->tmpbasename);
acfg->llvm_sfile = g_strdup_printf ("%s-llvm.s", acfg->tmpbasename);

Expand Down Expand Up @@ -15379,6 +15391,8 @@ set_paths (MonoAotCompile *acfg)
/* Done later */
} else {
acfg->tmpbasename = g_build_filename (acfg->aot_opts.temp_path, "temp", (const char*)NULL);
g_assert (acfg->tmpbasename);

acfg->asm_fname = g_strdup_printf ("%s.s", acfg->tmpbasename);
}
}
Expand Down Expand Up @@ -15624,6 +15638,7 @@ compile_assemblies_in_child (MonoAotOptions *aot_opts, MonoAssembly **assemblies

#ifdef HOST_WIN32
response_fname = g_build_filename (aot_opts->temp_path, "temp.rsp", (const char*)NULL);
g_assert (response_fname);
response = fopen (response_fname, "w");
g_assert (response);
#endif
Expand Down
3 changes: 3 additions & 0 deletions src/mono/mono/mini/mini-generic-sharing.c
Original file line number Diff line number Diff line change
Expand Up @@ -1312,6 +1312,7 @@ get_wrapper_shared_vtype (MonoType *t)

MonoClass *tuple_inst = mono_class_inflate_generic_class_checked (tuple_class, &ctx, error);
mono_error_assert_ok (error);
g_assert (tuple_inst);

//printf ("T: %s\n", mono_class_full_name (tuple_inst));

Expand Down Expand Up @@ -1411,6 +1412,7 @@ get_wrapper_shared_type_full (MonoType *t, gboolean is_field)
}
klass = mono_class_inflate_generic_class_checked (mono_class_get_generic_class (klass)->container_class, &ctx, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (klass);

t = m_class_get_byval_arg (klass);
MonoType *shared_type = get_wrapper_shared_vtype (t);
Expand Down Expand Up @@ -4349,6 +4351,7 @@ get_shared_type (MonoType *t, MonoType *type)

k = mono_class_inflate_generic_class_checked (gclass->container_class, &context, error);
mono_error_assert_ok (error); /* FIXME don't swallow the error */
g_assert (k);

return mini_get_shared_gparam (t, m_class_get_byval_arg (k));
} else if (MONO_TYPE_ISSTRUCT (type)) {
Expand Down
39 changes: 21 additions & 18 deletions src/mono/mono/mini/mini-runtime.c
Original file line number Diff line number Diff line change
Expand Up @@ -404,14 +404,17 @@ void *(mono_global_codeman_reserve) (int size)
global_codeman = mono_code_manager_new ();
else
global_codeman = mono_code_manager_new_aot ();
return mono_code_manager_reserve (global_codeman, size);
ptr = mono_code_manager_reserve (global_codeman, size);
}
else {
mono_jit_lock ();
ptr = mono_code_manager_reserve (global_codeman, size);
mono_jit_unlock ();
return ptr;
}

/* Virtually all call sites for this API assume it can't return NULL. */
g_assert (ptr);
return ptr;
}

/* The callback shouldn't take any locks */
Expand Down Expand Up @@ -2167,7 +2170,7 @@ mono_emit_jit_dump (MonoJitInfo *jinfo, gpointer code)
int i;

memset (&rec, 0, sizeof (rec));

// populating info relating debug methods
dmji = mono_debug_find_method (jinfo->d.method, NULL);

Expand Down Expand Up @@ -4540,20 +4543,20 @@ mini_llvm_init (void)
}

#ifdef ENSURE_PRIMARY_STACK_SIZE
/*++
Function:
EnsureStackSize
Abstract:
This fixes a problem on MUSL where the initial stack size reported by the
pthread_attr_getstack is about 128kB, but this limit is not fixed and
the stack can grow dynamically. The problem is that it makes the
functions ReflectionInvocation::[Try]EnsureSufficientExecutionStack
to fail for real life scenarios like e.g. compilation of corefx.
Since there is no real fixed limit for the stack, the code below
ensures moving the stack limit to a value that makes reasonable
real life scenarios work.
/*++
Function:
EnsureStackSize
Abstract:
This fixes a problem on MUSL where the initial stack size reported by the
pthread_attr_getstack is about 128kB, but this limit is not fixed and
the stack can grow dynamically. The problem is that it makes the
functions ReflectionInvocation::[Try]EnsureSufficientExecutionStack
to fail for real life scenarios like e.g. compilation of corefx.
Since there is no real fixed limit for the stack, the code below
ensures moving the stack limit to a value that makes reasonable
real life scenarios work.
--*/
static MONO_NO_OPTIMIZATION MONO_NEVER_INLINE void
ensure_stack_size (size_t size)
Expand Down Expand Up @@ -4737,7 +4740,7 @@ mini_init (const char *filename)
mono_w32handle_init ();
#endif

#ifdef ENSURE_PRIMARY_STACK_SIZE
#ifdef ENSURE_PRIMARY_STACK_SIZE
ensure_stack_size (5 * 1024 * 1024);
#endif // ENSURE_PRIMARY_STACK_SIZE

Expand Down
1 change: 1 addition & 0 deletions src/mono/mono/mini/mini.c
Original file line number Diff line number Diff line change
Expand Up @@ -4322,6 +4322,7 @@ mini_handle_call_res_devirt (MonoMethod *cmethod)

inst = mono_class_inflate_generic_class_checked (mono_class_get_iequatable_class (), &ctx, error);
mono_error_assert_ok (error);
g_assert (inst);

// EqualityComparer<T>.Default returns specific types depending on T
// FIXME: Special case more types: byte, string, nullable, enum ?
Expand Down
Loading

0 comments on commit 172ecdf

Please sign in to comment.