bitbuffer reads beyond the end?
23 views
Skip to first unread message
Greg Troxel
Sep 7, 2026, 9:43:54 AMSep 7
to rtl433
I have submitted a PR to improve EN2058 validation:
https://github.com/merbanan/rtl_433/pull/3686
As I read the code, bits are extracted from the bitbuffer withouut
checking that enough bits are actually present.
My first question is do people think am confused that code must check
for bits being present before extracting them.
My second question is that I think the bitbuffer decode routines should
internally check and assert, and this suggests that they aren't. Code
reading says they aren't. Am I confused here (too :-)?
https://github.com/merbanan/rtl_433/pull/3686
As I read the code, bits are extracted from the bitbuffer withouut
checking that enough bits are actually present.
My first question is do people think am confused that code must check
for bits being present before extracting them.
My second question is that I think the bitbuffer decode routines should
internally check and assert, and this suggests that they aren't. Code
reading says they aren't. Am I confused here (too :-)?
Christian Z.
Sep 7, 2026, 10:50:28 AMSep 7
to rtl_433
A decoder should not read too far past the end. There will always be some 0 bits, but e.g. at some later row there might only be 40 bytes remaining.
A length check after the bitbuffer_search() is missing here.
Also the e.g. "temperature2_C" key is wrong, it really should "temperature_2_C".
The last batch of decoders were not checked thoroughly. Comments and PRs welcome!
Greg Troxel
Sep 7, 2026, 3:34:38 PMSep 7
to Christian Z., rtl_433
"Christian Z." <chri...@zuckschwerdt.org> writes:
> A decoder should not read too far past the end. There will always be some 0
> bits, but e.g. at some later row there might only be 40 bytes remaining.
thanks; that's what I thought.
> A decoder should not read too far past the end. There will always be some 0
> bits, but e.g. at some later row there might only be 40 bytes remaining.
> A length check after the bitbuffer_search() is missing here.
> Also the e.g. "temperature2_C" key is wrong, it really should
> "temperature_2_C".
>
> The last batch of decoders were not checked thoroughly. Comments and PRs
> welcome!
Greg Troxel
Sep 7, 2026, 3:39:37 PMSep 7
to Christian Z., rtl_433
"Christian Z." <chri...@zuckschwerdt.org> writes:
> The last batch of decoders were not checked thoroughly. Comments and PRs
> welcome!
We probably should insist, when merging decoders, that they validate
> The last batch of decoders were not checked thoroughly. Comments and PRs
> welcome!
length (no unchecked reads), and that everything that is known is
checked. Trying to use the HA integration has pointed out that there
are way too many false decodes.
I would go further, and say that if any field is not decoded, the
decoder should be changed to output it as an unknown field, and the
submitter can figure out whether to leave it because it really varies,
or to check for a small set of values, if that seems ok.
Reply all
Reply to author
Forward
0 new messages