Fix blob pad

[edjin]

Hi!

I found the bug while trying to decode messages containing blobs. It happens that when a blob's size is divisible by 4, 4 more bytes than required are skipped.

[klingtnet]

Sorry that it took me almost a year to check your PR, and thanks for the contribution! I force-pushed the branch to rebase onto the latest state of master.

[klingtnet]

Okay, I now understand what was wrong with pad_to_32_bit_boundary. The function returned 4 instead of 0 for inputs whose length was a multiple of 4.
I wonder if we can fix this more generally, like in this patch:

commit 64072a489e0e8aac8dd818fb9bf5e39728f36191
Author: Andreas Linz <klingt.net@gmail.com>
Date:   2025-03-06 11:47:28 +0100

    fixup! Fix blob pad

diff --git a/src/decoder.rs b/src/decoder.rs
index afc5d3b..425ea8a 100644
--- a/src/decoder.rs
+++ b/src/decoder.rs
@@ -152,7 +152,7 @@ fn read_osc_string<'a>(
     map_res(
         terminated(
             take_till(|c| c == 0u8),
-            nullbit_then_pad_to_32_bit_boundary(original_input),
+            pad_to_32_bit_boundary(original_input),
         ),
         |str_buf: &'a [u8]| {
             String::from_utf8(str_buf.into())
@@ -282,17 +282,10 @@ fn pad_to_32_bit_boundary<'a>(
     original_input: &'a [u8],
 ) -> impl Fn(&'a [u8]) -> IResult<&'a [u8], (), OscError> {
     move |input| {
-        let offset = (4 - original_input.offset(input) % 4) % 4;
-        let (input, _) = take(offset)(input)?;
-        Ok((input, ()))
-    }
-}
-
-fn nullbit_then_pad_to_32_bit_boundary<'a>(
-    original_input: &'a [u8],
-) -> impl Fn(&'a [u8]) -> IResult<&'a [u8], (), OscError> {
-    move |input| {
-        let offset = 4 - original_input.offset(input) % 4;
+        let offset = match original_input.offset(input) % 4 {
+            0 => 0,
+            r => 4 - r % 4,
+        };
         let (input, _) = take(offset)(input)?;
         Ok((input, ()))
     }

But, this breaks two existing tests, test_bundle and test_encode_message_with_all_types for which I need to check if their assertions are expecting the right values.

[edjin]

Yess, I have to duplicate the function because the tests broke, and it doesn't seems the assertions are wrong.

For information, in the spec, string messages must end by the null byte, then completed with the padding, but blob don't end by this null byte.

Feel free to modify this code if you found a more convenient way to do that, ask me if you want me to make some changes.

[klingtnet]

In read_osc_string I think we need to take the input until we see a null-byte including the said null-byte, and then pad the matched sequence.
I failed to combine the parsers in a way that they also include the null-byte. Naively I thought that take_till(|c| c == 0u8).and_then(tag(b"\0")) would do the trick, but it doesn't.

[klingtnet]

I think with 8b6c8be I found a concise solution. There's still some potential for improvement, though.

[edjin]

This looks good/better to me, Thanks 👍

[IRSMsoso]

Awesome, thanks for merging