Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

segmentation fault in _default_material argument of create_structure_and_set_materials #1483

Closed
jianguan210 opened this issue Jan 26, 2021 · 5 comments · Fixed by #1583
Closed

Comments

@jianguan210
Copy link
Contributor

When I ran a simulation case involved gradient calculation (calculate_gradient()), default_material in C++ implementation is never assigned the type of material_type and pointed to a valid memory allocation in the workflow of gradient calculation. So the conversion from void* to material_type is invalid. A simple workaround is to delete the "else if" branch, and It passed test cases. Please check this issue.

@stevengj
Copy link
Collaborator

stevengj commented Feb 3, 2021

A simple workaround is to delete the "else if" branch

Which else if branch?

@stevengj
Copy link
Collaborator

stevengj commented Feb 3, 2021

I'm not clear on what simulation you are running — IIRC, anything that calls the routines in meepgeom.cpp should normally call set_materials_from_geometry first, which initializes default_material?

@oskooi
Copy link
Collaborator

oskooi commented May 5, 2021

To provide some additional information, the error reported by the address sanitizer is related to the statement (material_data *)default_material which occurs in three places within meepgeom.cpp:

meep/src/meepgeom.cpp

Lines 402 to 406 in f2bae0c

if (!tp && is_material_grid(default_material)) {
map_lattice_coordinates(p.x,p.y,p.z);
gradient = material_grid_grad(p, (material_data *)default_material);
++matgrid_val_count;
}

meep/src/meepgeom.cpp

Lines 444 to 448 in f2bae0c

if (!tp && is_material_grid(default_material)) {
map_lattice_coordinates(p.x,p.y,p.z);
u = material_grid_val(p, (material_data *)default_material);
if (matgrid_val_count == 0) udefault = u;
if (u < umin) umin = u;

meep/src/meepgeom.cpp

Lines 2549 to 2553 in f2bae0c

mg = (material_data *)tp->objects[oi].o->material;
else if (!tp && ((material_type)default_material)->which_subclass == material_data::MATERIAL_GRID)
mg = (material_data *)default_material;
else
return; /* no material grids at this point */

The issue here is that default_material is of type void * and in certain cases it is a NULL pointer (?) because its default value is nothing as defined in libctl:

(define nothing (if (eq? MATERIAL-TYPE 'SCM)
                   '()
                   (if (for-all? (class-properties-all material-type)
                                 property-has-default?)
                       (make material-type)
                       no-default)))

(define-input-var default-material nothing MATERIAL-TYPE)

A NULL pointer therefore cannot be cast into a material_data * which is where the error arises. Perhaps we should just check that default_material is not NULL as part of the assignment?

@oskooi oskooi changed the title Type conversion issue (void* to material_type) reported by ASan cannot cast NULL pointer to material_type for default material in meepgeom.cpp May 5, 2021
@stevengj
Copy link
Collaborator

It seems like there is a missing init_libctl call in _get_gradient? Also relevant to #1574.

@oskooi
Copy link
Collaborator

oskooi commented May 21, 2021

The problem here seems to be SWIG deleting the _default_material argument at the end of create_structure_and_set_materials. Inside this function, _default_material is passed to init_libctl which is then assigned to the global variable default_material:

meep/python/meep.i

Lines 1887 to 1889 in ef8f6fc

bool _ensure_periodicity,
meep_geom::material_type _default_material,
meep_geom::absorber_list alist,

meep/python/meep.i

Lines 1907 to 1908 in ef8f6fc

meep_geom::init_libctl(_default_material, _ensure_periodicity,
&gv, cell_size, center, &gobj_list);

meep/src/meepgeom.cpp

Lines 2060 to 2070 in ef8f6fc

void init_libctl(material_type default_mat, bool ensure_per, meep::grid_volume *gv,
vector3 cell_size, vector3 cell_center,
geometric_object_list *geom_) {
geom_initialize();
default_material = default_mat;
ensure_periodicity = ensure_per;
geometry_center = cell_center;
dimensions = meep::number_of_directions(gv->dim);
geometry_lattice.size = cell_size;
geom_fix_object_list(*geom_);
}

Note that _default_material is of type meep_geom::material_type which is a pointer to the struct material_data:

typedef material_data *material_type;

The SWIG generated file meep-python.cxx shows that the _default_material argument of create_structure_and_set_materials (arg18) is being deleted as part of the function SWIGINTERN PyObject *_wrap_create_structure_and_set_materials(PyObject *SWIGUNUSEDPARM(self), PyObject *args):

    if (arg18) {
      if (arg18->medium.E_susceptibilities.items) {
        delete[] arg18->medium.E_susceptibilities.items;
      }
      if (arg18->medium.H_susceptibilities.items) {
	delete[] arg18->medium.H_susceptibilities.items;
      }
      delete[] arg18->weights;
      delete[] arg18->epsilon_data;
      delete arg18;
    }

The obvious solution seems to be to just prevent SWIG from deleting _default_material. Putting a const in front of this argument in the function header for create_structure_and_set_materials doesn't do this. Do we need to somehow explicitly tell SWIG not to delete _default_material?

@oskooi oskooi changed the title cannot cast NULL pointer to material_type for default material in meepgeom.cpp segmentation fault in _default_material argument of create_structure_and_set_materials Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants