Fix a batch of Coverity Scan static analysis defects#5057

akohlmey · 2026-06-25T23:10:12Z

Summary

This pull request addresses a batch of defects reported by Coverity Scan static analysis. It contains a set of small, localized changes (each in its own commit), plus a Coverity modeling file and consolidated export tooling under tools/coverity/. The fixes were selected from a full triage of the open Coverity reports as the ones that are genuine and confidently fixable

Author(s)

Axel Kohlmeyer (Temple University), akohlmey@gmail.com (corresponding author)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Artificial Intelligence (AI) Tools Usage

The changes in this pull request were developed with the assistance of an AI tool (Anthropic Claude, Opus 4.8) operating under the direction and review of the author. The tool was used to triage the Coverity Scan defect reports, distinguish genuine issues from false positives, and draft the corresponding fixes; the author reviewed and verified every change. The individual commits carry a Co-Authored-By trailer reflecting this.

Implementation Notes

Tooling / infrastructure:

tools/coverity/coverity_model.cpp: a Coverity modeling file for the custom Memory allocator (models smalloc/srealloc/sfree as alloc/realloc/free).
tools/coverity/ export scripts + READMEs: local helper tooling to export the defect grid and per-defect event traces from the Coverity Scan web interface (the open-source tier offers no direct export).

Bug fixes (one commit each):

GRANSURF (pair_surf_granular, fix_surface_global, fix_surface_local): guard a negative contact-map index; make a non-exhaustive triangle corner-flag branch exhaustive; check the STL reader return value before using it as a size.
fix gemc: free the three RanPark RNGs in the destructor (leak) and initialize the pointer members.
RIGID (fix_rigid, fix_rigid_small): convert the readfile FILE* to SafeFilePtr (use-after-free on the error path), fill a molecule branch gap, null the bonus pointers in set_xv, and avoid dereferencing end iterators in rendezvous_body.
temper / temper/npt / temper/grem: validate the broadcast swap-partner index before using it to index the world arrays.
read_restart / variable: fix unterminated-string reads (use memcmp on the fixed-length magic string; bound and null-terminate the fread buffer).
neighbor: give the base create_kokkos_list an implementation that raises an internal error instead of silently doing nothing (clears an uninitialized-list false positive while encoding the invariant).
mliap_model: use SafeFilePtr for the coefficient file (RAII close on every path; removes the use-after-close and the exception-mode leak).
replicate: guard the newtag map lookups against a missing atom.
fix_phonon: initialize and range-check the Gauss-Jordan pivot indices (also yields a graceful singular-matrix error).
reader_native: reject a negative per-chunk element count when skipping a binary dump frame.

Verification: builds cleanly with the CMake build system (gcc + most presets); make check-whitespace and make check-permissions pass.

Post Submission Checklist

The feature or features in this pull request is complete
Licensing information is complete
Corresponding author information is complete
The source code follows the LAMMPS formatting guidelines
The feature has been verified to work with the CMake based build system

Add tools/coverity/coverity_model.cpp and a README documenting the model-file workflow. The model tells the Coverity analysis engine that Memory::smalloc/srealloc/sfree are allocate/reallocate/free, so the analysis stops reporting false "resource leak" and "use after free" defects that stem from it not recognizing the custom allocator. The templated Memory::create/grow/destroy helpers funnel through these three primitives, so modeling the primitives covers the whole allocator. Error::all/one/done are intentionally not modeled: they already carry the [[noreturn]] attribute, which cov-analyze honors directly (preferable to a model because it also informs the compiler and other tools). The file is not part of the build; it is uploaded to the Coverity Scan project. See tools/coverity/README.md for the cov-make-library build and upload workflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Coverity Scan flagged two negative-array-index reads in PairSurfGranular::calculate_3d_forces: contacts_map[contact_surfs[n].ck1] and contacts_map[contact_surfs[n].ck2] were indexed under only an "if (which1/which2 != -1)" guard. But ck1/ck2 are initialized to -1 and only overwritten when a matching contact is found, so contacts_map[-1] is reachable -- an out-of-bounds read whose result then indexes contact_surfs[]. The three analogous uses elsewhere in the same function already guard on "ck != -1"; add the same guard to these two sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

FixGEMC::FixGEMC allocates three RanPark RNGs (random_proc, random_world, random_universe) but the destructor freed only local_gas_list and the MPI communicator, leaking all three on every teardown (Coverity CTOR_DTOR_LEAK). Add the missing deletes. Also initialize the class pointer members (c_pe, local_gas_list, sublo, subhi, random_proc/world/universe) to nullptr in the constructor initializer list, per the LAMMPS coding standard (Coverity "Uninitialized pointer field"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Nine Coverity defects in the GRANSURF surface-granular contact code, located from their event traces: calculate_3d_forces (pair surf/granular and fix surface/global): * 6x "Uninitialized pointer read": pt/pt1/pt2 were assigned only in an if (flag==-4/-5/-6) chain with no else, so the analyzer saw an unhandled flag<=-7 path reaching &corners[..][pt]. The overlap classification is geometrically bounded to -1..-6 (the 3 edges and 3 corners of a triangle; see the contract documented above overlap_sphere_tri), so that path is unreachable. Make the chain exhaustive (else) to state the invariant and clear the warnings. * 1x "Negative array index read": connect3d[j].external_edge[which2] was reached with which2 == -1 only on the same unreachable flag<=-7 path; guard the access with which1 >= 0 && which2 >= 0. fix surface (extract_from_stlfile): * 1x "Overflowed integer argument" (a real bug): STLReader::read_file() can return a negative error code, which made ntris negative so that ntris*sizeof(Tri) underflowed to a huge srealloc size. Check the return value and error out. The remaining "Overflowed integer argument" in grow_connect (CID 503957) is already prevented by the existing bigint/MAXSMALLINT guard; no change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Combine the two local helper scripts (defect-grid export and per-defect trace export) into a single tools/coverity/coverity_export.py with "defects", "traces", and "self-test" subcommands, so the Coverity triage workflow is documented and reusable rather than living in throwaway scripts at the repo root. defects: paging on the open-source tier is stateful and split across two endpoints. POST /views/table.json {projectId,viewId,pageNum} selects the page server-side (keyed to the session cookie); GET /reports/table.json?projectId&viewId then returns that page's rows. The tool interleaves them on one session and walks every page until the whole set is collected. (Scraping the data GET alone, with an offset/pageNum in its URL, only ever returned the first page, because the page selector is the side-channel POST.) The XSRF-TOKEN cookie value is echoed in the X-XSRF-TOKEN header automatically (the endpoints' double-submit CSRF check). traces: GET /sourcebrowser/source.json keyed by fileInstanceId + defectInstanceId (from the grid export), filtering the file's events down to the requested defect. Both paths abort on an expired-session login response instead of saving it; on an unexpected layout the defects pager dumps the response shape rather than silently writing zero rows. Add README_export.md (cookie capture, the stateful two-endpoint paging, troubleshooting) and rename the existing README.md to README_modeling.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Five Coverity defects in the RIGID package, located from their event traces. readfile() in both files (Use after free x2): false positives. Coverity treats MPI_Bcast(&nlines) as possibly overwriting nlines, so on the rank-0 nlines==0 path (where fp had just been fclose'd) it walks past the "if (nlines==0) return" guard and reports the closed fp being passed to read_lines_from_file. Convert the FILE* to a SafeFilePtr (RAII): the file is closed automatically on every exit path -- early return, error throw, normal end -- so there is no manual fclose for the analyzer to misattribute, and the non-root fp is a clean nullptr instead of an uninitialized pointer. FixRigid() (Uninitialized pointer write): the v_ atom-variable branch filled molecule[i] only for in-group atoms, leaving the rest uninitialized; add the "else molecule[i] = 0" zero-fill that the i_ custom-property branch already has. set_xv() (Uninitialized pointer write): ebonus/lbonus/tbonus were assigned only under if(avec_*) but used in the matching eflags branch; initialize them to nullptr (the eflag implies the corresponding avec exists, so the access is never actually reached uninitialized -- this is defensive). rendezvous_body() (Using invalid iterator): the hash is built from in[].bodyID in the loop immediately above, so hash.find(in[i].bodyID) always succeeds; use hash[in[i].bodyID] (no end-iterator dereference) at all four lookup sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Three Coverity OVERRUN findings -- set_temp[partner_set_temp] reached via scale_velocities() in temper.cpp and temper_npt.cpp, and set_lambda[partner_set_lambda] in temper_grem.cpp -- are false positives. partner_set_temp is my_set_temp +/- 1, so it can be -1 or nworlds at the boundaries, but a swap is only ever accepted for an in-range partner (boundary and unpaired worlds end up with swap == 0), so the index is always valid wherever set_temp/set_lambda is actually read. That invariant spans the swap-result MPI_Bcast and the partner == -1 logic, which the analyzer cannot follow, so it sees the unclamped partner_set_temp flowing into the array. Make the invariant explicit (and the code robust) with a [[noreturn]] assertion right after the swap broadcast: if swap is set but the partner index is out of range -- which must not happen -- abort with an internal error instead of indexing out of bounds. Coverity then propagates the bound to the downstream accesses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Three Coverity STRING_NULL findings. read_restart.cpp magic_string() / check_eof_magic(): str is filled by fread/sfread of exactly n = strlen(MAGIC_STRING)+1 bytes and then compared with strcmp(str, MAGIC_STRING). That is actually safe -- the literal MAGIC_STRING is null-terminated at index n-1, so strcmp stops there and never reads past the n-byte buffer -- but it relies on that subtle bound. Use memcmp(str, MAGIC_STRING, n) instead: an exact n-byte compare with the same truth value and no dependence on str being null-terminated. variable.cpp next() (uloop/universe path): a genuine over-read. buf[64] had only its first two bytes zeroed before fread(buf,1,64,fp), so a normal lock file content such as "5\n" overwrites those two bytes and leaves buf[2..] uninitialized with no terminator; strlen(buf) then walks past the content into uninitialized stack, potentially past buf[63]. It happens to work because stoi stops at the newline and stack memory usually has a zero byte nearby, but it is undefined behavior. Read at most sizeof(buf)-1 bytes and null-terminate explicitly at buf[nread]. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Coverity flagged Neighbor::init_pair() using lists[i] uninitialized at neighbor.cpp:911. For a KOKKOS neighbor request the loop calls create_kokkos_list(i) instead of "lists[i] = new NeighList(lmp)", and the base-class create_kokkos_list was an empty {} stub, so the analyzer could not see lists[i] getting assigned on that branch. This is a false positive in practice: a KOKKOS request only exists when the neighbor object is a NeighborKokkos, whose override assigns lists[i] for exactly the kokkos_device / kokkos_host cases init_pair calls it for. But the safety rests entirely on that invariant, which the silent empty base stub does not encode. Replace the empty base stub with a real definition that errors out: reaching it means a KOKKOS neighbor list was requested without the KOKKOS package, which cannot happen. This documents the invariant, fails loudly instead of leaving lists[i] unset if it is ever violated, and -- error->all being [[noreturn]] -- lets the analyzer see that lists[i] is always assigned on any path that reaches its use. (Unlike the sibling init_*_kokkos no-op stubs, create_kokkos_list carries the responsibility of assigning lists[i], so it warrants the check.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

…effs read_coeffs() closed the coefficient FILE* early on EOF (inside the header and per-coeff read loops) and again at the normal end, relying on the intervening error->all() to terminate. Coverity flags the path where, if that error were to return, the next fgets() would use the closed handle (use-after-close); the same early-close pattern also leaked the handle if error->all() throws (the library exception build). Switch the handle to SafeFilePtr (RAII): it is nullptr on the non-root ranks that never open it, and auto-closes on every exit path -- normal return, EOF error, or exception -- so all three manual fclose() calls and the use-after-close go away. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

newtag() dereferenced old_map.find(tag)->second for the anchor atom and the bond/angle/dihedral/improper partner without checking the result against end(). old_map holds the entire original system, so for valid topology both tags are always present -- but a bond referencing a non-existent atom (corrupted data) would dereference the end iterator (undefined behavior; Coverity "Using invalid iterator"). Using operator[] would be wrong here (it would insert a bogus {tag,0} entry and silently use atom 0's coordinates), so check both iterators and raise a clear error instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

The complex Gauss-Jordan elimination set the pivot row/column (irow/icol) only inside the pivot-search branch, then used icol as an array index (ipiv[icol], later indxr/indxc) without a guaranteed initialization the analyzer could see (Coverity "uninitialized ... write"). The search always finds a pivot for a valid matrix, so this was safe in practice, but reset irow/icol to -1 at the start of each elimination step and raise the existing "Singular matrix" error if no pivot is found -- this both clears the finding and handles a degenerate matrix gracefully instead of indexing with a stale/garbage value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

ReaderNative::skip() reads a per-chunk element count n from the (binary) dump file and calls skip_buf(n*sizeof(double)). skip_buf() takes a size_t, so a negative n from a corrupted/invalid file is converted to a huge unsigned value (Coverity "Overflowed integer argument"). Add an n < 0 check that raises the same "Dump file is invalid or corrupted" error already used for the chunk count a few lines above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HrAVWctE6TRM5n875zpBnu

Add coverity_export.py plus README.md for offline triage of the free Coverity Scan results: it pages the defect grid and the per-defect event traces into JSON via the undocumented View-Defects endpoints, reusing a browser session cookie. Customized for LAMMPS-GUI (project 16333, view 67542, scan8 shard wired in as the built-in defaults so it runs without flags) from the LAMMPS tools/coverity script (PR lammps/lammps#5057). It is a standalone manual helper -- not wired into CTest and needs no build -- that complements the daily coverity-scan.yml upload workflow. The offline self-test (python coverity_export.py self-test) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Nrx1fTyQ8Zx9sTZnje5PP

stanmoore1

I approve, but had one comment.

stanmoore1 · 2026-07-01T14:59:51Z

        }
      }
    }
+    if (icol < 0) error->one(FLERR,"Singular matrix in complex GaussJordan!");


I don't think the exclamation point is needed here.

I don't think the exclamation point is needed here.

Looks like Claude was adapting to the general style of the source file which is "freckled with exclamation points" all over and some other things that we usually don't do in LAMMPS. I'm going to do a general exorcism.

I'm going to do a general exorcism.

I'm going to vote that we start adding more emojis to error messages in LAMMPS to make it more exciting when simulations crash.

"Non-numeric positions - simulation unstable :("

adding more emojis to error messages in LAMMPS

Nice idea!

But on the serious side, this only makes sense if we allow UTF-8 characters and that would open a can of worm that I prefer to leave closed until we have time to do a general audit of the core of LAMMPS to be able to handle this cleanly. We avoid a lot of trouble by sticking to (7-bit) ASCII and rather transparently transform UTF-8 lookalikes to their ASCII equivalents. BTW: this also gets us on the good side of software secrurity people(!).

"Non-numeric positions - simulation unstable :("

Wouldn't it be better to have: "Non-numeric positions - simulation unstable 🤯"

Or use 🤦 when people make impossible choices.

Wouldn't it be better to have: "Non-numeric positions - simulation unstable 🤯"

Or use 🤦 when people make impossible choices.

Could we have a randomly generated emoji to keep it fresh?

If you are going down this route you could go all the way and also do the same as GROMACS and Gaussian do and add a random "fortune" quote at the end of a run. But then again, we should aim for something different, so perhaps some ASCII-art images?
Curious to hear what @sjplimp thinks about this kind of stuff.

stanmoore1 · 2026-07-01T16:13:52Z

  if (narg < 8) error->all(FLERR,"Illegal fix phonon command: number of arguments < 8");

-  nevery = utils::inumeric(FLERR, arg[3],false,lmp);   // Calculate this fix every n steps!
+  nevery = utils::inumeric(FLERR, arg[3],false,lmp);   // Calculate this fix every n step!


I think the wrong char got removed here.

akohlmey and others added 13 commits June 25, 2026 09:10

akohlmey mentioned this pull request Jun 25, 2026

Initialize uninitialized class-member pointers in constructors (Coverity) #5058

Merged

5 tasks

akohlmey self-assigned this Jun 26, 2026

akohlmey added this to the Stable Release 20 August 2026 milestone Jun 26, 2026

akohlmey added this to LAMMPS Pull Requests Jun 26, 2026

akohlmey moved this to Ready for Review or Merge in LAMMPS Pull Requests Jun 26, 2026

akohlmey added bugfix refactor labels Jun 26, 2026

Merge branch 'develop' into coverity-bugfixes

d341011

akohlmey marked this pull request as ready for review June 26, 2026 04:37

akohlmey requested review from athomps, jtclemm and sjplimp as code owners June 26, 2026 04:37

akohlmey added maintenance and removed refactor labels Jun 26, 2026

akohlmey requested a review from stanmoore1 June 26, 2026 04:46

stanmoore1 approved these changes Jul 1, 2026

View reviewed changes

Convert Gilbert Gottfried style error messages to sound more like Steve

26a6a51

stanmoore1 self-requested a review July 1, 2026 16:13

stanmoore1 reviewed Jul 1, 2026

View reviewed changes

akohlmey added 2 commits July 1, 2026 18:11

general cleanup and improvement of error handling in fix phonon

08b84f9

Merge remote-tracking branch 'github/develop' into coverity-bugfixes

de09d91

akohlmey merged commit f7c9991 into lammps:develop Jul 1, 2026
9 checks passed

github-project-automation Bot moved this from Ready for Review or Merge to Done in LAMMPS Pull Requests Jul 1, 2026

akohlmey deleted the coverity-bugfixes branch July 1, 2026 22:59

akohlmey mentioned this pull request Jul 3, 2026

Coverity Scan triage part 1: bug fixes #5072

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix a batch of Coverity Scan static analysis defects#5057