I was checking some code[1], and found some null pointer deref and other precondition issues.
One interesting pattern was when the use happens in a constructor initializer list, but then the actual check is done in the constructor body.
Any opinions on what pattern should be used, here? One way is to simply move the usage below the check. But that prevents the compiler from enforcing e.g. that d_ted is initialized before d_interp, which uses d_ted, is initialized.
Another way is this:
$ git diff
diff --git a/gr-digital/lib/symbol_sync_ff_impl.cc b/gr-digital/lib/symbol_sync_ff_impl.cc
index 82b5a8036..dbc68a4f1 100644
--- a/gr-digital/lib/symbol_sync_ff_impl.cc
+++ b/gr-digital/lib/symbol_sync_ff_impl.cc
@@ -47,6 +47,15 @@ symbol_sync_ff::sptr symbol_sync_ff::make(enum ted_type detector_type,
taps);
}
+template <typename C, typename F>
+auto check(C c, F f)
+{
+ if (!c()) {
+ throw std::runtime_error("bleh");
+ }
+ return f();
+}
+
symbol_sync_ff_impl::symbol_sync_ff_impl(enum ted_type detector_type,
const float sps,
const float loop_bw,
@@ -69,7 +78,11 @@ symbol_sync_ff_impl::symbol_sync_ff_impl(enum ted_type detector_type,
damping_factor,
ted_gain),
d_interp(interpolating_resampler_fff::make(
- interp_type, d_ted->needs_derivative(), n_filters, taps)),
+ interp_type,
+ check([this] { return d_ted != nullptr; },
+ [this] { return d_ted->needs_derivative(); }),
+ n_filters,
+ taps)),
d_inst_output_period(sps / static_cast<float>(osps)),
d_inst_clock_period(sps),
d_avg_clock_period(sps),
diff --git a/gr-digital/lib/symbol_sync_ff_impl.cc b/gr-digital/lib/symbol_sync_ff_impl.cc
index 82b5a8036..dbc68a4f1 100644
--- a/gr-digital/lib/symbol_sync_ff_impl.cc
+++ b/gr-digital/lib/symbol_sync_ff_impl.cc
@@ -47,6 +47,15 @@ symbol_sync_ff::sptr symbol_sync_ff::make(enum ted_type detector_type,
taps);
}
+template <typename C, typename F>
+auto check(C c, F f)
+{
+ if (!c()) {
+ throw std::runtime_error("bleh");
+ }
+ return f();
+}
+
symbol_sync_ff_impl::symbol_sync_ff_impl(enum ted_type detector_type,
const float sps,
const float loop_bw,
@@ -69,7 +78,11 @@ symbol_sync_ff_impl::symbol_sync_ff_impl(enum ted_type detector_type,
damping_factor,
ted_gain),
d_interp(interpolating_resampler_fff::make(
- interp_type, d_ted->needs_derivative(), n_filters, taps)),
+ interp_type,
+ check([this] { return d_ted != nullptr; },
+ [this] { return d_ted->needs_derivative(); }),
+ n_filters,
+ taps)),
d_inst_output_period(sps / static_cast<float>(osps)),
d_inst_clock_period(sps),
d_avg_clock_period(sps),
But introducing such a pattern is a bigger stylistic change, and seems like it should be "blessed" as a good pattern before spreading.
This also applies to other UB stuff like glfsr_source_b_impl.cc which shift left by a potentially UB-triggering value before it bounds checks it (also it should not allow 64 exactly, and probably instead needs to special case it).
Opinions?
[1] Well, truth be told I'm asking LLM to "find bugs and UB".
typedef struct me_s {
char name[] = { "Thomas Habets" };
char email[] = { "thomas@habets.se" };
char kernel[] = { "Linux" };
char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt" };
char pgp[] = { "9907 8698 8A24 F52F 1C2E 87F6 39A4 9EEA 460A 0169" };
char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
} me_t;
char name[] = { "Thomas Habets" };
char email[] = { "thomas@habets.se" };
char kernel[] = { "Linux" };
char *pgpKey[] = { "http://www.habets.pp.se/pubkey.txt" };
char pgp[] = { "9907 8698 8A24 F52F 1C2E 87F6 39A4 9EEA 460A 0169" };
char coolcmd[] = { "echo '. ./_&. ./_'>_;. ./_" };
} me_t;
No comments:
Post a Comment