Add wrapper api which has 8-byte integer specific and another 4-byte integer specific (for better type safety)

[seanm]

I don't personally use seacas, but I've recently been trying to get all VTK unit tests passing with UBSan

One remaining thing flagged is:

VTK/ThirdParty/exodusII/vtkexodusII/src/ex_get_block.c:65:7: runtime error: store to misaligned address 0x7ff7bdd396c4 for type 'int64_t' (aka 'long long'), which requires 8 byte alignment

VTK's copy of seacas seems to be a year or two old, but the code in your current master looks unchanged at first glance.

Are your own unit tests ever built with UBSan?

[gsjaardema]

We do have a CI github actions build that uses UBSan. It does not seem to show this issue, but I don't know why... I will try a local UBSan build and see if I can reproduce.

[gsjaardema]

Is the above error caused while running the exodus regression tests, or a different test?

[seanm]

Many of the VTK tests with Exodus in the name fail with UBSan, for example:

	328 - VTK::IOIOSSCxx-TestIOSSExodus (ILLEGAL)
	329 - VTK::IOIOSSCxx-TestIOSSExodusMergeEntityBlocks (ILLEGAL)
	330 - VTK::IOIOSSCxx-TestIOSSExodusRestarts (ILLEGAL)
	331 - VTK::IOIOSSCxx-TestIOSSExodusSetArrays (ILLEGAL)
	332 - VTK::IOIOSSCxx-TestIOSSExodusWriterCrinkleClip (ILLEGAL)
	333 - VTK::IOIOSSCxx-TestIOSSExodusWriter (ILLEGAL)
	402 - VTK::IOExodusCxx-TestExodusAttributes (ILLEGAL)
	403 - VTK::IOExodusCxx-TestExodusIgnoreFileTime (ILLEGAL)
	404 - VTK::IOExodusCxx-TestExodusSideSets (ILLEGAL)
	405 - VTK::IOExodusCxx-TestMultiBlockExodusWrite (ILLEGAL)
	406 - VTK::IOExodusCxx-TestExodusTetra15 (ILLEGAL)
	407 - VTK::IOExodusCxx-TestExodusWedge18 (ILLEGAL)
	408 - VTK::IOExodusCxx-TestExodusWedge21 (ILLEGAL)
	409 - VTK::IOExodusCxx-Tetra15 (ILLEGAL)
	410 - VTK::IOExodusCxx-Wedge18 (ILLEGAL)
	411 - VTK::IOExodusCxx-Wedge21 (ILLEGAL)
	412 - VTK::IOExodusPython-TestExodusPolyhedra (Subprocess aborted)
	413 - VTK::IOExodusPython-TestExodusPolyhedraAgain (Subprocess aborted)
	414 - VTK::IOExodusPython-TestExodusWithNaN (Subprocess aborted)
	1464 - VTK::FiltersExtractionCxx-TestExtractExodusGlobalTemporalVariables (ILLEGAL)

If you try with VTK, be sure to use current master, because a build error with UBSan was recently fixed.

[gsjaardema]

Is this run on linux or windows or mac?

[gsjaardema]

Is this a problem with the exodus routine, or is it a problem with the calling routine which passes an unaligned block down into the function that needs 8-byte aligned data? The exodus routine is just filling the 8-byte integers with 8-byte integer data and not doing any of the memory allocation.

For example, is a 4-byte integer value being passed into a function that is expecting 8-byte integers? I don't see how else to get an unaligned address into the function...

[seanm]

Is this run on linux or windows or mac?

Mac.

Is this a problem with the exodus routine, or is it a problem with the calling routine

Dunno. But from the sounds of your analysis, it's looking like the latter I guess.

We do have a CI github actions build that uses UBSan.

That's good to know, thanks. It also suggests the issue may be in VTK itself.

I'll look a little more on the VTK side...

[seanm]

OK, for VTK's IOExodusCxx-TestExodusSideSets test UBSan flags here:

* thread #1, name = 'main thread', queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    frame #0: 0x00000001005b32f6 libvtkexodusII-9.2.1.dylib`vtkexodusII_ex_get_block(exoid=65536, blk_type=EX_EDGE_BLOCK, blk_id=100, entity_descrip="", num_entries_this_blk=0x00007ff7bfefde30, num_nodes_per_entry=0x00007ff7bfefded0, num_edges_per_entry=0x00007ff7bfefded4, num_faces_per_entry=0x00007ff7bfefded8, num_attr_per_entry=0x00007ff7bfefdee0) at ex_get_block.c:65:26
   62  	      *n_nodes_per_entry = block.num_nodes_per_entry;
   63  	    }
   64  	    if (n_edges_per_entry) {
-> 65  	      *n_edges_per_entry = block.num_edges_per_entry;
   66  	    }
   67  	    if (n_faces_per_entry) {
   68  	      *n_faces_per_entry = block.num_faces_per_entry;
Target 0: (vtkIOExodusCxxTests) stopped.

The problem is writing to n_edges_per_entry, which is the 3rd to last parameter. Going up one frame:

(lldb) fr sel 1
frame #1: 0x00000001008692ca libvtkIOExodus-9.2.1.dylib`vtkExodusIIReaderPrivate::RequestInformation(this=0x000000010b0051f0) at vtkExodusIIReader.cxx:4177:11
   4174	        }
   4175	        else
   4176	        {
-> 4177	          VTK_EXO_FUNC(ex_get_block(exoid, static_cast<ex_entity_type>(obj_types[i]), ids[obj],
   4178	                         obj_typenames[obj], &binfo.Size, &binfo.BdsPerEntry[0],
   4179	                         &binfo.BdsPerEntry[1], &binfo.BdsPerEntry[2], &binfo.AttributesPerEntry),
   4180	            "Could not read block params.");

3rd to last param is &binfo.BdsPerEntry[1] and BdsPerEntry is declared as int not int64_t: https://gitlab.kitware.com/vtk/vtk/-/blob/master/IO/Exodus/vtkExodusIIReaderPrivate.h#L360

So this is only working at all since most CPUs are little endian these days.

If ex_get_block's params were declared as int64_t* instead of void_int * the compiler would probably have warned...

[seanm]

VTK patch: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10053

[gsjaardema]

If ex_get_block's params were declared as int64_t* instead of void_int * the compiler would probably have warned...

Yes, that is a disadvantage of the method that is used for 8/4 byte integers and 8/4 byte floats. It would be good to maybe have an additional wrapper api which was 8-byte integer specific and another 4-byte integer specific. I wouldn't replace the current api since it works very well in codes that can switch 4/8-byte integer/float at runtime...

[seanm]

That does sound like a good idea. Shall we just rename this issue for that purpose?

[gsjaardema]

It might be good to have that as an issue. I don't know when I would be able to get to it, but maybe someone would see it and decide it is a good way to learn the exodus api and submit a PR... Or, maybe I will find time at some point.