GNU bug report logs - #55719
29.0.50; various bindat strz bugs

Previous Next

Full log

Message #22 received at 55719 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 55719 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: [PATCH] bindat strz fixes
Date: Wed, 1 Jun 2022 01:28:06 -0400

[Message part 1 (text/plain, inline)]

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 (text/x-patch, attachment)]

[0002-bindat-strz-Fix-off-by-one-bug-in-computed-length.patch (text/x-patch, attachment)]

[0003-bindat-strz-Consistent-length-type-check.patch (text/x-patch, attachment)]

[0004-bindat-strz-Always-write-null-terminator.patch (text/x-patch, attachment)]

[0005-bindat-strz-Fix-wrong-type-argument-error-when-unpac.patch (text/x-patch, attachment)]

[0006-bindat-strz-Move-all-pack-logic-to-pack-function.patch (text/x-patch, attachment)]

[0007-bindat-strz-Fix-packing-of-long-strings-with-legacy-.patch (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.