Re: [PATCH] bindat strz fixes

[Richard Hansen]

Thank you for the review! I have attached a new set of patches.

On 5/31/22 19:00, Stefan Monnier wrote:
> > +(let ((spec (bindat-type :pack-var v
> > +                         (x strz 2 :pack-val v)
> > +                         :unpack-val x)))
>
> Any particular reason why you define it this way instead of just
>
>      (bindat-type strz 2)
>
> ?

Thanks, I should have realized that was possible. Fixed.

> > +  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
> > +    :expected-result :failed
> > +    (should (equal (bindat-pack spec "abc") "\141\0")))
>
> I think this changes the intended semantics.  Until now `strz N` has
> meant that N bytes are used to encode the string and that it can
> hold upto a string of length N (in which case there's no terminating NUL
> byte).  I agree that it's not the only valid semantics, but I'm not sure
> we want to change it at this point.
>
> Do you have a particular reason to make this change.

A few:
  * The documentation says that the packed output is null terminated
    so that's what users expect.
  * It is safer (packed output is less likely to cause some other
    program to run past the end of the field).
  * Without this change, there is no difference between `strz N` and
    `str N`. So what would be the point of `strz N`?
  * If the user selected strz, the application probably requires null
    termination in all cases, not just when the string is under a
    certain length.

> > +  (ert-deftest bindat-test--strz-fixedlen-unpack ()
> > +    (should (equal (bindat-unpack spec "\0\0") ""))
> > +    (should (equal (bindat-unpack spec "a\0") "a"))))
>
> How 'bout
>
>       (bindat-unpack spec "ab")
>
> ?

I added some comments explaining why cases like that aren't tested.

> (tho I'd write "abc\0" i.s.o "\141\142\143\0").

Done.

> Not sure what we should do about (bindat-unpack spec "abc")?

That is not a valid packed string, so I'm not too concerned about it.
Currently it signals an args-out-of-range error because it tries to
read past the end of the string, which seems like an acceptable
behavior to me. We could make the error message more friendly, such as
"end of input reached while searching for null terminator", but I'd
rather not make that change right now with this set of patches.

> > -     `(cl-incf bindat-idx ,(cond
> > -                            ((null len) `(length ,val))
> > -                            ((numberp len) len)
> > -                            (t `(or ,len (length ,val))))))
> > +     `(cl-incf bindat-idx ,(if (numberp len)
> > +                               len
> > +                             `(1+ (length ,val)))))
>
> `len` is supposed to be an ELisp *expression*.

Ah, I was wondering why it was written that way. Fixed.

0001-bindat-tests-strz-Add-more-tests.patch
Description: Text Data

0002-bindat-strz-Fix-off-by-one-bug-in-computed-length.patch
Description: Text Data

0003-bindat-strz-Consistent-length-type-check.patch
Description: Text Data

0004-bindat-strz-Always-write-null-terminator.patch
Description: Text Data

0005-bindat-strz-Fix-wrong-type-argument-error-when-unpac.patch
Description: Text Data

0006-bindat-strz-Move-all-pack-logic-to-pack-function.patch
Description: Text Data

0007-bindat-strz-Fix-packing-of-long-strings-with-legacy-.patch
Description: Text Data

OpenPGP_signature
Description: OpenPGP digital signature