Skip to content

Fix signedness warning in data section size check (fixes #486) - #487

Merged
horgh merged 1 commit into
maxmind:mainfrom
robert-scheck:uint64_t
Sep 13, 2026
Merged

horgh merged 1 commit into
maxmind:mainfrom
robert-scheck:uint64_t

Conversation

@robert-scheck

@robert-scheck robert-scheck commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Fix a -Wsign-compare warning in the data section size check:

src/maxminddb.c:370:27: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]

data_section_size is a ssize_t, while UINT32_MAX is unsigned. The check is adjusted to avoid the signed/unsigned comparison. For consistency, the <= 0 check is performed first so the value is known to be positive before converting it to uint64_t.

Summary by CodeRabbit

  • Refactor
    • Reordered validation checks for data section size without changing behavior, error handling, or user-visible functionality.

src/maxminddb.c:370:27: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘int’} and ‘unsigned int’ [-Werror=sign-compare]
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a29a2843-1aa2-4041-abf3-36c89c976d12

📥 Commits

Reviewing files that changed from the base of the PR and between f42d97b and cd138a6.

📒 Files selected for processing (1)
  • src/maxminddb.c

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change reorders two validation operands in MMDB_open. Error handling and control flow remain unchanged.

Changes

Metadata validation

Layer / File(s) Summary
Reorder metadata size checks
src/maxminddb.c
The validation checks data_section_size <= 0 before the UINT32_MAX comparison. Both invalid cases return MMDB_INVALID_METADATA_ERROR and jump to cleanup.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Bug fix

Suggested reviewers: oschwald

Merge Risk: ⚪ Minimal · up to cd138

This change only reorders equivalent validation checks and carries no identified merge risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: correcting a signedness warning in the data section size check. It also references the related issue.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit checks the size with care
The smaller test now leads the pair
The error path stays in place
No change disturbs the cleanup race
The code hops on, neat and bright

Comment @coderabbitai help to get the list of available commands.

@horgh
horgh merged commit 00f59ab into maxmind:main Sep 13, 2026
20 checks passed
@horgh

horgh commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Thank you!

horgh added a commit that referenced this pull request Sep 13, 2026
ssize_t and long are 32 bits wide on i686. That exposes signedness and
range problems that the 64-bit jobs cannot see, such as the
-Wsign-compare error fixed in #487, which only showed up in Fedora's
i686 build.

The new job builds and tests with -m32 using gcc and clang. The flags
are set for the whole job so that the libtap build, which has its own
Makefile, and the test scripts that compile code themselves pick them
up.

The decoder limits test kept only -f flags from the environment when it
rebuilt the library, so it would have dropped -m32 and checked 64-bit
code. It now keeps -m flags as well. With the #487 fix reverted, the old
script passed at 32-bit and the new one fails with the original error.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@robert-scheck
robert-scheck deleted the uint64_t branch September 13, 2026 18:49
oschwald added a commit that referenced this pull request Sep 14, 2026
Add changelog entry for #487 and fix a comment typo
pull Bot pushed a commit to parhelia512/libmaxminddb that referenced this pull request Sep 14, 2026
The fix in maxmind#487 resolved a build failure for packagers on platforms
where ssize_t is 32 bits, such as i686, where -Werror turns the
-Wsign-compare warning into an error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants