lib: unpack size_t values safely by verma-divyanshu-git · Pull Request #569 · facebook/bpfilter

verma-divyanshu-git · 2026-07-17T15:02:07Z

Summary

add a typed size_t unpack helper while preserving the uint64_t wire format
reject packed values above SIZE_MAX and update the affected unpack call sites
add focused unit coverage for normal and overflowing values

Fixes #548

meta-cla · 2026-07-17T15:02:13Z

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

qdeslandes · 2026-07-17T16:42:54Z

Hi, could you please disclose if/how much AI has been used for this work? Thank you.

verma-divyanshu-git · 2026-07-17T17:10:11Z

Hi @qdeslandes , I used AI to help with the initial understanding & investigation (searching the relevant unpack call sites etc) and used it to draft an initial version of the patch.

I reviewed and edited the diff, reproduced the incompatible-pointer-type issue from the reported 32-bit build failure, verified the size_t to uint64_t conversion and overflow behavior across the affected unpack sites, and I own the final submission.

qdeslandes · 2026-07-17T17:24:04Z

Hi @qdeslandes , I used AI to help with the initial understanding & investigation (searching the relevant unpack call sites etc) and used it to draft an initial version of the patch.

I reviewed and edited the diff, reproduced the incompatible-pointer-type issue from the reported 32-bit build failure, verified the size_t to uint64_t conversion and overflow behavior across the affected unpack sites, and I own the final submission.

Thank you for this answer.

Did you build and run the code? Did you run the tests? Did you sign the CLA?

verma-divyanshu-git · 2026-07-17T20:42:40Z

Hi @qdeslandes , I used AI to help with the initial understanding & investigation (searching the relevant unpack call sites etc) and used it to draft an initial version of the patch.
I reviewed and edited the diff, reproduced the incompatible-pointer-type issue from the reported 32-bit build failure, verified the size_t to uint64_t conversion and overflow behavior across the affected unpack sites, and I own the final submission.

Thank you for this answer.

Did you build and run the code? Did you run the tests? Did you sign the CLA?

Yes. I built and tested the updated branch in a fresh privileged Fedora 44 environment.

Results:

build completed successfully
unit tests: 23/23 passed
e2e tests: 93/93 passed
integration tests: 2/2 passed
formatting/static checks: 4/4 passed
coverage target completed successfully

The CLA is signed as well.

verma-divyanshu-git · 2026-07-24T20:32:59Z

@qdeslandes The CLA check is green and the branch is up to date with main.

When you have a chance, could you please review the patch?

@sudipm-mukherjee, if convenient, could you confirm that the current branch resolves the i386 package build reported in #548?

verma-divyanshu-git · 2026-07-30T09:53:06Z

hi @qdeslandes, could you please take another look when you have a chance? the cla is green, the branch is mergeable, and all local checks passed; if everything looks good, could you please merge it? thank you.

sudipm-mukherjee · 2026-07-30T11:29:21Z

@sudipm-mukherjee, if convenient, could you confirm that the current branch resolves the i386 package build reported in #548?

sorry was busy with $dayjob. I will try to check it tonight.

qdeslandes

LGTM, except for a couple of nits:

This PR could be a single commit (72 chars wide)
Remove the merge commit

qdeslandes · 2026-08-04T13:33:19Z

+    assert_ok(bf_rpack_new(&rpack, data, data_len));
+    root = bf_rpack_root(rpack);
+
+    assert_int_equal(bf_rpack_kv_size(root, "size_val", &size_val), -EOVERFLOW);


assert_err would be better, so we ensure the call fails, but we don't freeze a specific errno value.

fixed in 8bfb0ad. the overflow test now uses assert_err.

qdeslandes · 2026-08-04T13:40:12Z

@claude review

github-actions · 2026-08-04T19:29:18Z

Claude review of PR #569 (`8bfb0ad`)

Suggestions

Guard tautological SIZE_MAX comparison — src/libbpfilter/pack.c:546 — On 64-bit, _value > SIZE_MAX is always false and may trigger -Wtype-limits; wrap with #if SIZE_MAX < UINT64_MAX like the test already does

Nits

(None beyond what the maintainer already flagged: use assert_err in the overflow test, squash to single commit)

CLAUDE.md improvements

(None identified)

Workflow run

verma-divyanshu-git · 2026-08-12T13:29:48Z

@qdeslandes : updated in 8bfb0ad: rebased on current main and squashed the patch to one commit with no merge commits.

qdeslandes

Last nit, then LGTM.

qdeslandes · 2026-08-13T12:24:53Z

+#if SIZE_MAX < UINT64_MAX
+static void rpack_size_t_overflow(void **state)
+{
+    _free_bf_wpack_ bf_wpack_t *wpack = NULL;
+    _free_bf_rpack_ bf_rpack_t *rpack = NULL;
+    const void *data;
+    size_t data_len;
+    bf_rpack_node_t root;
+    size_t size_val = 0;
+
+    (void)state;
+
+    assert_ok(bf_wpack_new(&wpack));
+    bf_wpack_kv_u64(wpack, "size_val", (uint64_t)SIZE_MAX + 1);
+    assert_ok(bf_wpack_get_data(wpack, &data, &data_len));
+
+    assert_ok(bf_rpack_new(&rpack, data, data_len));
+    root = bf_rpack_root(rpack);
+
+    assert_err(bf_rpack_kv_size(root, "size_val", &size_val));
+}
+#endif


Move if SIZE_MAX < UINT64_MAX within the function's body, so it (the check) can be removed from the tests array.

done in 7f5362e. the guard is now inside the test body.

qdeslandes · 2026-08-13T12:25:32Z

@claude review

Read packed sizes through uint64_t to preserve the wire format. Reject values that do not fit in size_t on 32-bit targets.

verma-divyanshu-git · 2026-08-19T22:46:37Z

@qdeslandes is it ready to be merged now?

verma-divyanshu-git requested a review from qdeslandes as a code owner July 17, 2026 15:02

meta-cla Bot added the cla signed label Jul 17, 2026

verma-divyanshu-git force-pushed the fix-548-i386-size-t-unpack branch from 08a14cb to 3f6db8a Compare July 17, 2026 20:40

qdeslandes requested changes Aug 4, 2026

View reviewed changes

github-actions Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread src/libbpfilter/pack.c

verma-divyanshu-git force-pushed the fix-548-i386-size-t-unpack branch from ed5232e to 8bfb0ad Compare August 12, 2026 13:29

qdeslandes requested changes Aug 13, 2026

View reviewed changes

lib: pack: unpack size_t without truncation

7f5362e

Read packed sizes through uint64_t to preserve the wire format. Reject values that do not fit in size_t on 32-bit targets.

verma-divyanshu-git force-pushed the fix-548-i386-size-t-unpack branch from 8bfb0ad to 7f5362e Compare August 15, 2026 21:18

verma-divyanshu-git requested a review from qdeslandes August 20, 2026 09:04

Uh oh!

Conversation

verma-divyanshu-git commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

meta-cla Bot commented Jul 17, 2026

Action Required

Process

Uh oh!

qdeslandes commented Jul 17, 2026

Uh oh!

verma-divyanshu-git commented Jul 17, 2026

Uh oh!

qdeslandes commented Jul 17, 2026

Uh oh!

verma-divyanshu-git commented Jul 17, 2026

Uh oh!

verma-divyanshu-git commented Jul 24, 2026

Uh oh!

verma-divyanshu-git commented Jul 30, 2026

Uh oh!

sudipm-mukherjee commented Jul 30, 2026

Uh oh!

qdeslandes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qdeslandes Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

verma-divyanshu-git Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

qdeslandes commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude review of PR #569 (8bfb0ad)

Suggestions

Nits

CLAUDE.md improvements

Uh oh!

Uh oh!

verma-divyanshu-git commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qdeslandes left a comment

Choose a reason for hiding this comment

Uh oh!

qdeslandes Aug 13, 2026

Choose a reason for hiding this comment

Uh oh!

verma-divyanshu-git Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qdeslandes commented Aug 13, 2026

Uh oh!

verma-divyanshu-git commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

verma-divyanshu-git commented Jul 17, 2026 •

edited

Loading

qdeslandes left a comment •

edited

Loading

github-actions Bot commented Aug 4, 2026 •

edited

Loading

Claude review of PR #569 (`8bfb0ad`)

verma-divyanshu-git commented Aug 12, 2026 •

edited

Loading

verma-divyanshu-git Aug 15, 2026 •

edited

Loading