[Bug report] `SamplerCombiner` ignores its second sampler parameter and returns incorrect combined values for distinct samplers

Bug Reports
1

Hi OpenFHE team,

I would like to report a reproducible correctness bug in the public SamplerCombiner class declared in src/core/include/math/discretegaussiangeneratorgeneric.h.

Summary

SamplerCombiner is documented and declared as a combiner for two samplers:

SamplerCombiner(BaseSampler* s1, BaseSampler* s2, int64_t z1, int64_t z2)

and GenerateInteger() computes:

x1 * sampler1->GenerateInteger() + x2 * sampler2->GenerateInteger()

However, the constructor currently initializes both stored sampler pointers from s1:

: sampler1(s1), sampler2(s1), x1(z1), x2(z2) {}

So s2 is ignored entirely. When a caller passes distinct samplers, the combined sample is computed as if both inputs were s1.

I reproduced this directly with a minimal public-header probe using two fixed samplers that return different integers. The expected combined value was 21, but the actual result was 11, which matches the mistaken use of s1 twice.

Current in-tree use does not dynamically expose this because the generic sampler constructor currently calls:

new SamplerCombiner(wide_sampler, wide_sampler, x1, x2);

with the same pointer in both sampler slots. So this is best described as a latent but real public-header correctness bug: external callers, tests, or future internal uses that pass distinct samplers will get the wrong result.

Environment

  • OpenFHE tested revision: ed361af22049007db2107e7c69bcff209e8c420d
  • OS: Linux x86_64
  • Compiler: Clang 14.0.0
  • Library configuration used for the dynamic probe:
    BUILD_SHARED=ON, BUILD_STATIC=OFF, MATHBACKEND=4, NATIVE_SIZE=128
  • The defect itself is in the header-defined constructor and does not appear specific to the 128-bit/backend-4 configuration; the configuration above is simply the build on which I ran the probe.

Minimal reproduction

The following probe uses only public core headers and constructs two trivial samplers returning fixed values 1 and 2. It then combines them with coefficients 1 and 10.

Mathematically, the expected result is:

1 * 1 + 10 * 2 = 21

#include "math/discretegaussiangeneratorgeneric.h"

#include <cstdint>
#include <iostream>

using namespace lbcrypto;

class FixedSampler final : public BaseSampler {
public:
    explicit FixedSampler(int64_t v) : value(v) {}

    int64_t GenerateInteger() override {
        return value;
    }

private:
    int64_t value;
};

int main() {
    FixedSampler first(1);
    FixedSampler second(2);

    SamplerCombiner combiner(&first, &second, 1, 10);
    const int64_t observed = combiner.GenerateInteger();
    const int64_t expected = 21;

    std::cout << "first=" << first.GenerateInteger() << '\n';
    std::cout << "second=" << second.GenerateInteger() << '\n';
    std::cout << "expected=" << expected << '\n';
    std::cout << "observed=" << observed << '\n';
    std::cout << "matches_expected=" << (observed == expected ? 1 : 0) << '\n';

    return observed == expected ? 0 : 1;
}

Representative probe build:

clang++-14 -std=gnu++17 -fno-omit-frame-pointer -g -O1 \
  openfhe_samplercombiner_probe.cpp \
  -Iopenfhe-development/src/core/include \
  -Iopenfhe-development/third-party/include \
  -Iopenfhe-development/third-party/cereal/include \
  -Ibuild-openfhe-128-nosan/src/core \
  -Lbuild-openfhe-128-nosan/lib \
  -Wl,-rpath,build-openfhe-128-nosan/lib \
  -lOPENFHEcore \
  -lpthread -ldl -lm \
  -o openfhe_samplercombiner_probe

Run:

./openfhe_samplercombiner_probe

Actual behavior

On my setup, the probe printed:

first=1
second=2
expected=21
observed=11
matches_expected=0

So the second sampler contributes as if it were the first sampler.

The observed result matches the current constructor exactly:

1 * first + 10 * first = 11

Expected behavior

If SamplerCombiner accepts two sampler pointers s1 and s2, it should use those two samplers as provided.

For the repro above, the result should therefore be:

1 * first + 10 * second = 21

Cause analysis

The current public class definition is:

class SamplerCombiner final : public BaseSampler {
public:
    SamplerCombiner(BaseSampler* s1, BaseSampler* s2, int64_t z1, int64_t z2)
        : sampler1(s1), sampler2(s1), x1(z1), x2(z2) {}

    int64_t GenerateInteger() override {
        return x1 * sampler1->GenerateInteger() + x2 * sampler2->GenerateInteger();
    }
    ...
};

The constructor documentation distinguishes:

  • s1: first sampler
  • s2: second sampler

but the implementation ignores s2 and stores s1 twice. As a result, the implementation obtains both terms through s1 rather than obtaining one term through s1 and the other through s2.

I only found one current in-tree call site:

wide_sampler = new SamplerCombiner(wide_sampler, wide_sampler, x1, x2);

Because both sampler arguments are the same there, the current generic-sampler construction path does not expose the bug dynamically. But the constructor is publicly declared in a core header, so external callers and future internal uses can observe the wrong behavior immediately when passing distinct samplers.

Impact

The confirmed impact is an incorrect combined sample when SamplerCombiner is constructed with distinct sampler pointers.

For distinct samplers, the intended computation draws one value from s1 and one value from s2. The current implementation instead obtains both terms from s1 through two separate GenerateInteger() calls. Therefore, when the two samplers have different output distributions, the resulting combined distribution can differ from the intended one, not merely an individual sample
value.

This is a correctness bug, not a memory-safety bug. I have not established a current top-level OpenFHE workflow that dynamically reaches it with distinct samplers, because the present in-tree call site passes the same sampler twice.

So the most accurate classification is:

  • correctness bug in a publicly accessible core-header class
  • currently latent in the main generic-sampler construction path
  • directly reproducible for external or future internal callers using the documented two-sampler interface

Relevant source locations

Suggested direction

The constructor should initialize the second stored sampler from s2:

SamplerCombiner(BaseSampler* s1, BaseSampler* s2, int64_t z1, int64_t z2)
    : sampler1(s1), sampler2(s2), x1(z1), x2(z2) {}

Question

If distinct sampler inputs are intended, would you accept changing sampler2(s1) to sampler2(s2) together with a small regression test for the distinct-sampler case?

Reported by Jiang Chao, Beijing University of Posts and Telecommunications

2

Thank you for reporting it. Added a new issue.