Fix CFITSIO safety issues: nullptr params, error handling, nrow clamping, CI coverage by asgr · Pull Request #28 · asgr/Rfits

asgr · 2026-04-01T02:42:29Z

Addresses several safety and correctness issues flagged in code review on the CFITSIO/Rcpp interface.

`src/Rfits.cpp`

fits_get_hdrpos nullptr — CFITSIO's position out-parameter is not optional; passing nullptr risks a segfault. Added int keypos at all 4 call sites and pass &keypos.
fits_close_file status ignored — Move-assignment and operator=(fitsfile*) silently dropped close errors. Both now check status and throw via fits_throw_exception. Move-assignment is no longer noexcept.
fits_get_colname unhandled errors — Loop was treating all non-COL_NOT_FOUND statuses as success, risking UB from uninitialized colname. Now pushes only on status == 0; throws on any other real error code.
nrow out-of-range — Changed from warning-and-proceed (which would still error/corrupt in CFITSIO) to clamping nrow = nrow_total - startrow + 1.
ensure_lossless_32bit_int indentation — Fallback memcpy/attr/return block was missing braces and misaligned; re-indented for unambiguous control flow.

`.github/workflows/main.yml`

Removed branches filter from push trigger so CI runs on all branches, not just main/master.
Restored {os: ubuntu-latest, r: 'devel'} to the matrix for forward-compatibility coverage.

Now there are no more recommended fixes (or possible corner case bugs) highlighted.

Bump Rfits package version to 1.14.5 and update release date

Copilot

Pull request overview

This PR focuses on making the CFITSIO/Rcpp interface code in Rfits.cpp safer by tightening ownership semantics, reducing manual memory management, and improving integer handling (especially around 32-bit vs 64-bit boundaries).

Changes:

Hardened FITS file handle ownership via non-copyable fits_file with move semantics and safer reassignment behavior.
Reduced heap churn / improved safety in several read paths (e.g., TSTRING column reads, header raw reads) using RAII containers.
Updated CI configuration and a couple of exported function signatures (Cfits_read_nrow, Cfits_write_col) to use long.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File	Description
src/Rfits.cpp	Safety/RAII improvements, integer range handling, API tweaks, and various read/write path refactors.
src/RcppExports.cpp	Regenerated/updated export signatures to match `long`-typed C++ APIs.
R/Rfits_methods_file.R	Minor whitespace cleanup.
.github/workflows/main.yml	CI trigger/matrix adjustments and dependency install tweaks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T02:48:01Z

src/Rfits.cpp

  fits_invoke(open_image, fptr, filename.get_cstring(), READONLY);
  fits_invoke(movabs_hdu, fptr, ext, &hdutype);
-  fits_invoke(get_hdrpos, fptr, &nkeys, &keypos);
+  fits_invoke(get_hdrpos, fptr, &nkeys, nullptr);



fits_get_hdrpos expects a valid int* for the current header position; passing nullptr risks a null dereference inside CFITSIO. Keep a local keypos variable (even if unused) and pass &keypos (or switch to fits_get_hdrspace with a real nmore variable).

Copilot · 2026-04-01T02:48:02Z

src/Rfits.cpp

  fits_invoke(open_image, fptr, filename.get_cstring(), READONLY);
  fits_invoke(movabs_hdu, fptr, ext, &hdutype);
-  fits_invoke(get_hdrpos, fptr, &nkeys, &keypos);
+  fits_invoke(get_hdrpos, fptr, &nkeys, nullptr);


Same issue here: fits_get_hdrpos's position out-parameter is not optional in the CFITSIO API; passing nullptr may crash. Preserve a keypos local and pass &keypos.

Copilot · 2026-04-01T02:48:02Z

src/Rfits.cpp

  fits_invoke(open_image, fptr, filename.get_cstring(), READWRITE);
  fits_invoke(movabs_hdu, fptr, ext, &hdutype);
-  fits_invoke(get_hdrpos, fptr, &nkeys, &keypos);
+  fits_invoke(get_hdrpos, fptr, &nkeys, nullptr);


Same fits_get_hdrpos null out-parameter problem: passing nullptr for the position argument is unsafe and can segfault inside CFITSIO. Pass a real int keypos instead.

Copilot · 2026-04-01T02:48:02Z

src/Rfits.cpp

  fits_file fptr;
  fits_invoke(open_image, fptr, filename.get_cstring(), READONLY);
  fits_invoke(movabs_hdu, fptr, ext, &hdutype);
-  fits_invoke(get_hdrpos, fptr, &nkeys, &keypos);
+  fits_invoke(get_hdrpos, fptr, &nkeys, nullptr);


Same fits_get_hdrpos usage issue: the third parameter is an output pointer (position) and CFITSIO's declaration does not indicate it can be null. Passing nullptr here can lead to a crash; pass &keypos.

Copilot · 2026-04-01T02:48:02Z

src/Rfits.cpp

  if (startrow + nrow - 1 > nrow_total) {
-    Rcpp::stop("Requested range exceeds number of rows in table");
+    Rcpp::warning("Requested range exceeds number of rows in table");
  }


This switches the out-of-range condition from stop() to warning() but still proceeds to call fits_read_col with the original nrow. That will typically still error in CFITSIO (or return partial/undefined results). Either keep this as a hard error, or clamp nrow to nrow_total - startrow + 1 before reading.

Copilot · 2026-04-01T02:48:03Z

src/Rfits.cpp

+  fits_file &operator=(fits_file &&other) noexcept {
+    if (this != &other) {
+      if (m_fptr) {
+        int status = 0;
+        fits_close_file(m_fptr, &status);
+      }
+      m_fptr = other.m_fptr;
+      other.m_fptr = nullptr;
+    }


When replacing an owned handle (both in move-assignment and operator=(fitsfile*)), fits_close_file's status is ignored. If the close fails (e.g., write/flush error), this will be silently dropped. Consider checking status here and surfacing it via fits_throw_exception("close_file", status) (safe to throw here; only the destructor should suppress close errors).

@copilot apply changes based on this feedback

Applied in commit 4abb4ab. Both operator=(fits_file&&) (also removed noexcept) and operator=(fitsfile*) now check the status returned by fits_close_file and call fits_throw_exception("close_file", status) if it fails. The destructor continues to suppress close errors as intended.

Copilot · 2026-04-01T02:48:03Z

src/Rfits.cpp

-	 return *this;
+    if (m_fptr) {
+      int status = 0;
+      fits_close_file(m_fptr, &status);


operator=(fitsfile*) closes the existing file but ignores the returned status. If close fails, the error is lost and the code proceeds to overwrite m_fptr. Consider checking status and reporting it (throw) before taking ownership of new_fptr (only the destructor should ignore close errors).

Suggested change

fits_close_file(m_fptr, &status);

fits_close_file(m_fptr, &status);

if (status) {

// Report close error before taking ownership of the new pointer.

fits_throw_exception("close_file", status);

}

Copilot · 2026-04-01T02:48:03Z

src/Rfits.cpp

+               [](long v) { return static_cast<int64_t>(v); });
+if (n > 0) std::memcpy(&(output[0]), &(values64[0]), n * sizeof(int64_t));
+output.attr("class") = "integer64";
+return output;


In ensure_lossless_32bit_int, the fallback block has inconsistent indentation and multiple statements on the same indentation level as the surrounding loop (e.g., the memcpy, attr, and return lines). Please re-indent and add braces so the control flow is unambiguous and maintainable.

Suggested change

[](long v) { return static_cast<int64_t>(v); });

if (n > 0) std::memcpy(&(output[0]), &(values64[0]), n * sizeof(int64_t));

output.attr("class") = "integer64";

return output;

[](long v) { return static_cast<int64_t>(v); });

if (n > 0) {

std::memcpy(&(output[0]), &(values64[0]), n * sizeof(int64_t));

}

output.attr("class") = "integer64";

return output;

Copilot · 2026-04-01T02:48:03Z

.github/workflows/main.yml

+    branches: [main, master]
  pull_request:
    branches: [main, master]



Adding push.branches: [main, master] will stop CI from running on pushes to any other branches (feature branches, release branches, etc.). If that’s not intended, remove the branch filter or broaden it (e.g., include development/release patterns).

Suggested change

branches: [main, master]

pull_request:

branches: [main, master]

pull_request:

branches: [main, master]

Copilot · 2026-04-01T02:48:04Z

.github/workflows/main.yml

          - {os: windows-latest,  r: 'release'}
-          - {os: ubuntu-latest,   r: 'devel', http-user-agent: 'release'}
          - {os: ubuntu-latest,   r: 'release'}
          - {os: ubuntu-latest,   r: 'oldrel-1'}


The matrix no longer runs ubuntu-latest with r: devel. This reduces forward-compatibility coverage and can delay detection of upcoming R changes. If CI time isn’t the driver, consider keeping the devel job or gating it (e.g., scheduled/weekly) instead of removing it entirely.

Suggested change

- {os: ubuntu-latest, r: 'oldrel-1'}

- {os: ubuntu-latest, r: 'oldrel-1'}

- {os: ubuntu-latest, r: 'devel'}

…ssignment operators Agent-Logs-Url: https://github.com/asgr/Rfits/sessions/cd685580-08b6-4b59-8870-dfb15131a891 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>

…name errors, indentation, CI fixes Agent-Logs-Url: https://github.com/asgr/Rfits/sessions/90408e21-b322-4cb6-aad4-c67c5025252c Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>

asgr added 18 commits March 18, 2026 15:28

major fixes

7dca650

Minor changes

d6630ef

Some more optimisations

19092b8

Compiles fine

1d0993f

Clean up

daf7000

Final fresh pass and some last tidy ups. Compiles fine (fewer warnings).

ca1f4f8

try now

f6fb2c8

Type fix

b448202

Safe

4067ea9

Now?

ba8867f

Ignore devel version

ea96187

Comment to stop future edits of this bit

82b3505

More fixes

75b52d9

Final tidy up.

e3add52

Now there are no more recommended fixes (or possible corner case bugs) highlighted.

Just run workflow on main and master

b497e7d

Merge pull request #27 from asgr/master

97f656c

Bump Rfits package version to 1.14.5 and update release date

Minor edits (esp since I never actually used Cfits_read_nrow)

bc5142d

Change to warning

cda45b4

asgr requested a review from Copilot April 1, 2026 02:42

Copilot started reviewing on behalf of asgr April 1, 2026 02:42 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Copilot started work on behalf of asgr April 1, 2026 02:53 View session

Fix fits_close_file status ignored in move-assignment and fitsfile* a…

4abb4ab

…ssignment operators Agent-Logs-Url: https://github.com/asgr/Rfits/sessions/cd685580-08b6-4b59-8870-dfb15131a891 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>

Copilot finished work on behalf of asgr April 1, 2026 02:58

Copilot started work on behalf of asgr April 1, 2026 05:46 View session

Address remaining PR review comments: hdrpos nullptr, nrow clamp, col…

d3e7438

…name errors, indentation, CI fixes Agent-Logs-Url: https://github.com/asgr/Rfits/sessions/90408e21-b322-4cb6-aad4-c67c5025252c Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>

Copilot AI changed the title ~~Test claude~~ Fix CFITSIO safety issues: nullptr params, error handling, nrow clamping, CI coverage Apr 1, 2026

Copilot finished work on behalf of asgr April 1, 2026 05:53

asgr added 2 commits April 1, 2026 14:33

Some final edits

c398e30

Get rid of failing Ubuntu (seems to be their issue, not mine)

cdf5b3f

asgr merged commit 8f8aac7 into master Apr 1, 2026
8 checks passed

-      fits_close_file(m_fptr, &status);
+      fits_close_file(m_fptr, &status);
+      if (status) {
+        // Report close error before taking ownership of the new pointer.
+        fits_throw_exception("close_file", status);
+      }

	- {os: ubuntu-latest, r: 'oldrel-1'}
	- {os: ubuntu-latest, r: 'oldrel-1'}
	- {os: ubuntu-latest, r: 'devel'}

Conversation

asgr commented Apr 1, 2026 • edited by Copilot AI Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

src/Rfits.cpp

.github/workflows/main.yml

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

asgr Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asgr commented Apr 1, 2026 •

edited by Copilot AI

Loading

`src/Rfits.cpp`

`.github/workflows/main.yml`