* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
@ 2022-06-17 22:52 Richard Hansen
2022-06-18 3:02 ` Richard Hansen
0 siblings, 1 reply; 6+ messages in thread
From: Richard Hansen @ 2022-06-17 22:52 UTC (permalink / raw)
To: 56048; +Cc: monnier
[-- Attachment #1.1.1: Type: text/plain, Size: 575 bytes --]
X-Debbugs-CC: monnier@iro.umontreal.ca
Two patches attached:
Patch 1:
; bindat (strz): Move all pack logic to pack function
Patch 2:
bindat (strz): Null terminate fixed-length strings if there is room
* lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
fields, explicitly write a null terminator after the packed string if
there is room.
* doc/lispref/processes.texi (Bindat Types): Update documentation.
* test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
Update tests.
[-- Attachment #1.1.2: 0001-bindat-strz-Move-all-pack-logic-to-pack-function.patch --]
[-- Type: text/x-patch, Size: 3731 bytes --]
From 062de6aae2e37110ac37557058d1738d009cdd77 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH 1/2] ; bindat (strz): Move all pack logic to pack function
---
lisp/emacs-lisp/bindat.el | 48 ++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 46e2a4901c..f7f540f5fd 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -440,20 +440,26 @@ bindat--pack-str
(aset bindat-raw (+ bindat-idx i) (aref v i)))
(setq bindat-idx (+ bindat-idx len))))
-(defun bindat--pack-strz (v)
+(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
- (len (length v)))
- (dotimes (i len)
- (when (= (aref v i) 0)
- ;; Alternatively we could pretend that this was the end of
- ;; the string and stop packing, but then bindat-length would
- ;; need to scan the input string looking for a null byte.
- (error "Null byte encountered in input strz string"))
- (aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided a
- ;; pre-allocated string to bindat-pack that wasn't zeroed first.
- (aset bindat-raw (+ bindat-idx len) 0)
- (setq bindat-idx (+ bindat-idx len 1))))
+ (vlen (length v)))
+ (if len
+ ;; When len is specified, behave the same as the str type since we don't
+ ;; actually add the terminating zero anyway (because we rely on the fact
+ ;; that `bindat-raw' was presumably initialized with all-zeroes before
+ ;; we started).
+ (bindat--pack-str len v)
+ (dotimes (i vlen)
+ (when (= (aref v i) 0)
+ ;; Alternatively we could pretend that this was the end of
+ ;; the string and stop packing, but then bindat-length would
+ ;; need to scan the input string looking for a null byte.
+ (error "Null byte encountered in input strz string"))
+ (aset bindat-raw (+ bindat-idx i) (aref v i)))
+ ;; Explicitly write a null terminator in case the user provided a
+ ;; pre-allocated string to bindat-pack that wasn't zeroed first.
+ (aset bindat-raw (+ bindat-idx vlen) 0)
+ (setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
(let ((bnum (1- (* 8 len))) j m)
@@ -482,7 +488,8 @@ bindat--pack-item
('u24r (bindat--pack-u24r v))
('u32r (bindat--pack-u32r v))
('bits (bindat--pack-bits len v))
- ((or 'str 'strz) (bindat--pack-str len v))
+ ('str (bindat--pack-str len v))
+ ('strz (bindat--pack-strz len v))
('vec
(let ((l (length v)) (vlen 1))
(if (consp vectype)
@@ -699,18 +706,7 @@ bindat--type
((numberp len) len)
;; General expression support.
(t `(or ,len (1+ (length ,val)))))))
- (`(pack . ,args)
- ;; When len is specified, behave the same as the str type since we don't
- ;; actually add the terminating zero anyway (because we rely on the fact
- ;; that `bindat-raw' was presumably initialized with all-zeroes before we
- ;; started).
- (cond ; Same optimizations as 'length above.
- ((null len) `(bindat--pack-strz . ,args))
- ((numberp len) `(bindat--pack-str ,len . ,args))
- (t (macroexp-let2 nil len len
- `(if ,len
- (bindat--pack-str ,len . ,args)
- (bindat--pack-strz . ,args))))))))
+ (`(pack . ,args) `(bindat--pack-strz ,len . ,args))))
(cl-defmethod bindat--type (op (_ (eql 'bits)) len)
(bindat--pcase op
--
2.36.1
[-- Attachment #1.1.3: 0002-bindat-strz-Null-terminate-fixed-length-strings-if-t.patch --]
[-- Type: text/x-patch, Size: 5315 bytes --]
From 325100cd7da32e9a13bc94a544896b69824aa12e Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 16 Jun 2022 15:21:57 -0400
Subject: [PATCH 2/2] bindat (strz): Null terminate fixed-length strings if
there is room
* lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
fields, explicitly write a null terminator after the packed string if
there is room.
* doc/lispref/processes.texi (Bindat Types): Update documentation.
* test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
Update tests.
---
doc/lispref/processes.texi | 30 ++++++++++++++--------------
lisp/emacs-lisp/bindat.el | 14 ++++++-------
test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
3 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index b9200aedde..d038d52d44 100644
--- a/doc/lispref/processes.texi
+++ b/doc/lispref/processes.texi
@@ -3509,23 +3509,23 @@ Bindat Types
(but excluding) the null byte that terminated the input string.
If @var{len} is provided, @code{strz} behaves the same as @code{str},
-but with one difference: when unpacking, the first null byte
-encountered in the packed string is interpreted as the terminating
-byte, and it and all subsequent bytes are excluded from the result of
-the unpacking.
+but with a couple of differences:
+
+@itemize @bullet
+@item
+When packing, a null terminator is written after the packed string if
+the length of the input string is less than @var{len}.
+
+@item
+When unpacking, the first null byte encountered in the packed string
+is interpreted as the terminating byte, and it and all subsequent
+bytes are excluded from the result of the unpacking.
+@end itemize
@quotation Caution
-The packed output will not be null-terminated unless one of the
-following is true:
-@itemize
-@item
-The input string is shorter than @var{len} bytes and either no pre-allocated
-string was provided to @code{bindat-pack} or the appropriate byte in
-the pre-allocated string was already null.
-@item
-The input string contains a null byte within the first @var{len}
-bytes.
-@end itemize
+The packed output will not be null-terminated unless the input string
+is shorter than @var{len} bytes or it contains a null byte within the
+first @var{len} bytes.
@end quotation
@item vec @var{len} [@var{type}]
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index f7f540f5fd..6428b0e965 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -443,11 +443,14 @@ bindat--pack-str
(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
(vlen (length v)))
+ ;; Explicitly write a null terminator (if there's room) in case the user
+ ;; provided a pre-allocated string to `bindat-pack' that wasn't already
+ ;; zeroed.
+ (when (or (null len) (< vlen len))
+ (aset bindat-raw (+ bindat-idx vlen) 0))
(if len
- ;; When len is specified, behave the same as the str type since we don't
- ;; actually add the terminating zero anyway (because we rely on the fact
- ;; that `bindat-raw' was presumably initialized with all-zeroes before
- ;; we started).
+ ;; When len is specified, behave the same as the str type (except for
+ ;; the null terminator possibly written above).
(bindat--pack-str len v)
(dotimes (i vlen)
(when (= (aref v i) 0)
@@ -456,9 +459,6 @@ bindat--pack-strz
;; need to scan the input string looking for a null byte.
(error "Null byte encountered in input strz string"))
(aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided a
- ;; pre-allocated string to bindat-pack that wasn't zeroed first.
- (aset bindat-raw (+ bindat-idx vlen) 0)
(setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index cc223ad14e..0c03c51e2e 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -172,14 +172,14 @@ bindat-test--str-strz-prealloc
((((x str 2)) ((x . "a"))) . "ax")
((((x str 2)) ((x . "ab"))) . "ab")
((((x str 2)) ((x . "abc"))) . "ab")
- ((,(bindat-type strz 1) "") . "xx")
- ((,(bindat-type strz 2) "") . "xx")
- ((,(bindat-type strz 2) "a") . "ax")
+ ((,(bindat-type strz 1) "") . "\0x")
+ ((,(bindat-type strz 2) "") . "\0x")
+ ((,(bindat-type strz 2) "a") . "a\0")
((,(bindat-type strz 2) "ab") . "ab")
((,(bindat-type strz 2) "abc") . "ab")
- ((((x strz 1)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . "a"))) . "ax")
+ ((((x strz 1)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . "a"))) . "a\0")
((((x strz 2)) ((x . "ab"))) . "ab")
((((x strz 2)) ((x . "abc"))) . "ab")
((,(bindat-type strz) "") . "\0x")
--
2.36.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
2022-06-17 22:52 bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room Richard Hansen
@ 2022-06-18 3:02 ` Richard Hansen
2022-06-18 6:32 ` Eli Zaretskii
2022-06-21 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
0 siblings, 2 replies; 6+ messages in thread
From: Richard Hansen @ 2022-06-18 3:02 UTC (permalink / raw)
To: 56048; +Cc: monnier
[-- Attachment #1.1.1: Type: text/plain, Size: 170 bytes --]
Attached are new revisions of the patches. The only differences are the comments were filled at column 70 instead of 80, and the commit message mentions the bug number.
[-- Attachment #1.1.2: v2-0001-bindat-strz-Move-all-pack-logic-to-pack-function.patch --]
[-- Type: text/x-patch, Size: 3748 bytes --]
From 6096bc8bceada87a5c54e4eb500812a0b72ffd44 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH v2 1/2] ; bindat (strz): Move all pack logic to pack function
---
lisp/emacs-lisp/bindat.el | 49 ++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 26 deletions(-)
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 46e2a4901c..4a642bb9c5 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -440,20 +440,27 @@ bindat--pack-str
(aset bindat-raw (+ bindat-idx i) (aref v i)))
(setq bindat-idx (+ bindat-idx len))))
-(defun bindat--pack-strz (v)
+(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
- (len (length v)))
- (dotimes (i len)
- (when (= (aref v i) 0)
- ;; Alternatively we could pretend that this was the end of
- ;; the string and stop packing, but then bindat-length would
- ;; need to scan the input string looking for a null byte.
- (error "Null byte encountered in input strz string"))
- (aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided a
- ;; pre-allocated string to bindat-pack that wasn't zeroed first.
- (aset bindat-raw (+ bindat-idx len) 0)
- (setq bindat-idx (+ bindat-idx len 1))))
+ (vlen (length v)))
+ (if len
+ ;; When len is specified, behave the same as the str type
+ ;; since we don't actually add the terminating zero anyway
+ ;; (because we rely on the fact that `bindat-raw' was
+ ;; presumably initialized with all-zeroes before we started).
+ (bindat--pack-str len v)
+ (dotimes (i vlen)
+ (when (= (aref v i) 0)
+ ;; Alternatively we could pretend that this was the end of
+ ;; the string and stop packing, but then bindat-length would
+ ;; need to scan the input string looking for a null byte.
+ (error "Null byte encountered in input strz string"))
+ (aset bindat-raw (+ bindat-idx i) (aref v i)))
+ ;; Explicitly write a null terminator in case the user provided
+ ;; a pre-allocated string to `bindat-pack' that wasn't already
+ ;; zeroed.
+ (aset bindat-raw (+ bindat-idx vlen) 0)
+ (setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
(let ((bnum (1- (* 8 len))) j m)
@@ -482,7 +489,8 @@ bindat--pack-item
('u24r (bindat--pack-u24r v))
('u32r (bindat--pack-u32r v))
('bits (bindat--pack-bits len v))
- ((or 'str 'strz) (bindat--pack-str len v))
+ ('str (bindat--pack-str len v))
+ ('strz (bindat--pack-strz len v))
('vec
(let ((l (length v)) (vlen 1))
(if (consp vectype)
@@ -699,18 +707,7 @@ bindat--type
((numberp len) len)
;; General expression support.
(t `(or ,len (1+ (length ,val)))))))
- (`(pack . ,args)
- ;; When len is specified, behave the same as the str type since we don't
- ;; actually add the terminating zero anyway (because we rely on the fact
- ;; that `bindat-raw' was presumably initialized with all-zeroes before we
- ;; started).
- (cond ; Same optimizations as 'length above.
- ((null len) `(bindat--pack-strz . ,args))
- ((numberp len) `(bindat--pack-str ,len . ,args))
- (t (macroexp-let2 nil len len
- `(if ,len
- (bindat--pack-str ,len . ,args)
- (bindat--pack-strz . ,args))))))))
+ (`(pack . ,args) `(bindat--pack-strz ,len . ,args))))
(cl-defmethod bindat--type (op (_ (eql 'bits)) len)
(bindat--pcase op
--
2.36.1
[-- Attachment #1.1.3: v2-0002-bindat-strz-Null-terminate-fixed-length-strings-i.patch --]
[-- Type: text/x-patch, Size: 5277 bytes --]
From 9ebda68264adca6f60f780d44995c4213d2c12c2 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 16 Jun 2022 15:21:57 -0400
Subject: [PATCH v2 2/2] bindat (strz): Null terminate fixed-length strings if
there is room
* lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
fields, explicitly write a null terminator after the packed string if
there is room (bug#56048).
* doc/lispref/processes.texi (Bindat Types): Update documentation.
* test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
Update tests.
---
doc/lispref/processes.texi | 30 ++++++++++++++--------------
lisp/emacs-lisp/bindat.el | 13 ++++++------
test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
3 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index b9200aedde..d038d52d44 100644
--- a/doc/lispref/processes.texi
+++ b/doc/lispref/processes.texi
@@ -3509,23 +3509,23 @@ Bindat Types
(but excluding) the null byte that terminated the input string.
If @var{len} is provided, @code{strz} behaves the same as @code{str},
-but with one difference: when unpacking, the first null byte
-encountered in the packed string is interpreted as the terminating
-byte, and it and all subsequent bytes are excluded from the result of
-the unpacking.
+but with a couple of differences:
+
+@itemize @bullet
+@item
+When packing, a null terminator is written after the packed string if
+the length of the input string is less than @var{len}.
+
+@item
+When unpacking, the first null byte encountered in the packed string
+is interpreted as the terminating byte, and it and all subsequent
+bytes are excluded from the result of the unpacking.
+@end itemize
@quotation Caution
-The packed output will not be null-terminated unless one of the
-following is true:
-@itemize
-@item
-The input string is shorter than @var{len} bytes and either no pre-allocated
-string was provided to @code{bindat-pack} or the appropriate byte in
-the pre-allocated string was already null.
-@item
-The input string contains a null byte within the first @var{len}
-bytes.
-@end itemize
+The packed output will not be null-terminated unless the input string
+is shorter than @var{len} bytes or it contains a null byte within the
+first @var{len} bytes.
@end quotation
@item vec @var{len} [@var{type}]
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 4a642bb9c5..0ecac3d52a 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -443,11 +443,14 @@ bindat--pack-str
(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
(vlen (length v)))
+ ;; Explicitly write a null terminator (if there's room) in case
+ ;; the user provided a pre-allocated string to `bindat-pack' that
+ ;; wasn't already zeroed.
+ (when (or (null len) (< vlen len))
+ (aset bindat-raw (+ bindat-idx vlen) 0))
(if len
;; When len is specified, behave the same as the str type
- ;; since we don't actually add the terminating zero anyway
- ;; (because we rely on the fact that `bindat-raw' was
- ;; presumably initialized with all-zeroes before we started).
+ ;; (except for the null terminator possibly written above).
(bindat--pack-str len v)
(dotimes (i vlen)
(when (= (aref v i) 0)
@@ -456,10 +459,6 @@ bindat--pack-strz
;; need to scan the input string looking for a null byte.
(error "Null byte encountered in input strz string"))
(aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided
- ;; a pre-allocated string to `bindat-pack' that wasn't already
- ;; zeroed.
- (aset bindat-raw (+ bindat-idx vlen) 0)
(setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index cc223ad14e..0c03c51e2e 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -172,14 +172,14 @@ bindat-test--str-strz-prealloc
((((x str 2)) ((x . "a"))) . "ax")
((((x str 2)) ((x . "ab"))) . "ab")
((((x str 2)) ((x . "abc"))) . "ab")
- ((,(bindat-type strz 1) "") . "xx")
- ((,(bindat-type strz 2) "") . "xx")
- ((,(bindat-type strz 2) "a") . "ax")
+ ((,(bindat-type strz 1) "") . "\0x")
+ ((,(bindat-type strz 2) "") . "\0x")
+ ((,(bindat-type strz 2) "a") . "a\0")
((,(bindat-type strz 2) "ab") . "ab")
((,(bindat-type strz 2) "abc") . "ab")
- ((((x strz 1)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . "a"))) . "ax")
+ ((((x strz 1)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . "a"))) . "a\0")
((((x strz 2)) ((x . "ab"))) . "ab")
((((x strz 2)) ((x . "abc"))) . "ab")
((,(bindat-type strz) "") . "\0x")
--
2.36.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
2022-06-18 3:02 ` Richard Hansen
@ 2022-06-18 6:32 ` Eli Zaretskii
2022-06-18 22:57 ` Richard Hansen
2022-06-21 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2022-06-18 6:32 UTC (permalink / raw)
To: Richard Hansen; +Cc: 56048, monnier
> Cc: monnier@iro.umontreal.ca
> Date: Fri, 17 Jun 2022 23:02:57 -0400
> From: Richard Hansen <rhansen@rhansen.org>
>
> Attached are new revisions of the patches. The only differences are the comments were filled at column 70 instead of 80, and the commit message mentions the bug number.
While waiting for Stefan to tell whether he has any comments, a couple
of nits:
> Subject: [PATCH v2 1/2] ; bindat (strz): Move all pack logic to pack function
What is the motivation/rationale for this refactoring?
> +When packing, a null terminator is written after the packed string if
> +the length of the input string is less than @var{len}.
Since "length of a string" is highly ambiguous in Emacs, please always
make a point of saying "@var{len} bytes" explicitly. Byte length is
something rarely seen or used in Emacs, so people must be informed
about that each time.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
2022-06-18 6:32 ` Eli Zaretskii
@ 2022-06-18 22:57 ` Richard Hansen
0 siblings, 0 replies; 6+ messages in thread
From: Richard Hansen @ 2022-06-18 22:57 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 56048, monnier
[-- Attachment #1.1.1: Type: text/plain, Size: 1171 bytes --]
On 2022-06-18 02:32, Eli Zaretskii wrote:
>> Subject: [PATCH v2 1/2] ; bindat (strz): Move all pack logic to pack function
>
> What is the motivation/rationale for this refactoring?
The attached revision updates the commit message to explain:
; bindat (strz): Move all pack logic to pack function
Motivation/rationale:
* Improve code readability. Now `bindat--pack-strz` is used for all
`strz` packing, not just variable-length `strz` packing.
* Make it easier to change the behavior of fixed-length `strz`
packing without also affecting the behavior of `str` packing. (A
future commit will modify `strz` to write a null terminator if
there is room.)
>
>> +When packing, a null terminator is written after the packed string if
>> +the length of the input string is less than @var{len}.
>
> Since "length of a string" is highly ambiguous in Emacs, please always
> make a point of saying "@var{len} bytes" explicitly. Byte length is
> something rarely seen or used in Emacs, so people must be informed
> about that each time.
Done; see attached revision.
Thanks,
Richard
[-- Attachment #1.1.2: v3-0001-bindat-strz-Move-all-pack-logic-to-pack-function.patch --]
[-- Type: text/x-patch, Size: 4124 bytes --]
From cbd138671c371e9919a2ed345d3579a8a163abc8 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH v3 1/2] ; bindat (strz): Move all pack logic to pack function
Motivation/rationale:
* Improve code readability. Now `bindat--pack-strz` is used for all
`strz` packing, not just variable-length `strz` packing.
* Make it easier to change the behavior of fixed-length `strz`
packing without also affecting the behavior of `str` packing. (A
future commit will modify `strz` to write a null terminator if
there is room.)
---
lisp/emacs-lisp/bindat.el | 49 ++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 26 deletions(-)
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 46e2a4901c..4a642bb9c5 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -440,20 +440,27 @@ bindat--pack-str
(aset bindat-raw (+ bindat-idx i) (aref v i)))
(setq bindat-idx (+ bindat-idx len))))
-(defun bindat--pack-strz (v)
+(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
- (len (length v)))
- (dotimes (i len)
- (when (= (aref v i) 0)
- ;; Alternatively we could pretend that this was the end of
- ;; the string and stop packing, but then bindat-length would
- ;; need to scan the input string looking for a null byte.
- (error "Null byte encountered in input strz string"))
- (aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided a
- ;; pre-allocated string to bindat-pack that wasn't zeroed first.
- (aset bindat-raw (+ bindat-idx len) 0)
- (setq bindat-idx (+ bindat-idx len 1))))
+ (vlen (length v)))
+ (if len
+ ;; When len is specified, behave the same as the str type
+ ;; since we don't actually add the terminating zero anyway
+ ;; (because we rely on the fact that `bindat-raw' was
+ ;; presumably initialized with all-zeroes before we started).
+ (bindat--pack-str len v)
+ (dotimes (i vlen)
+ (when (= (aref v i) 0)
+ ;; Alternatively we could pretend that this was the end of
+ ;; the string and stop packing, but then bindat-length would
+ ;; need to scan the input string looking for a null byte.
+ (error "Null byte encountered in input strz string"))
+ (aset bindat-raw (+ bindat-idx i) (aref v i)))
+ ;; Explicitly write a null terminator in case the user provided
+ ;; a pre-allocated string to `bindat-pack' that wasn't already
+ ;; zeroed.
+ (aset bindat-raw (+ bindat-idx vlen) 0)
+ (setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
(let ((bnum (1- (* 8 len))) j m)
@@ -482,7 +489,8 @@ bindat--pack-item
('u24r (bindat--pack-u24r v))
('u32r (bindat--pack-u32r v))
('bits (bindat--pack-bits len v))
- ((or 'str 'strz) (bindat--pack-str len v))
+ ('str (bindat--pack-str len v))
+ ('strz (bindat--pack-strz len v))
('vec
(let ((l (length v)) (vlen 1))
(if (consp vectype)
@@ -699,18 +707,7 @@ bindat--type
((numberp len) len)
;; General expression support.
(t `(or ,len (1+ (length ,val)))))))
- (`(pack . ,args)
- ;; When len is specified, behave the same as the str type since we don't
- ;; actually add the terminating zero anyway (because we rely on the fact
- ;; that `bindat-raw' was presumably initialized with all-zeroes before we
- ;; started).
- (cond ; Same optimizations as 'length above.
- ((null len) `(bindat--pack-strz . ,args))
- ((numberp len) `(bindat--pack-str ,len . ,args))
- (t (macroexp-let2 nil len len
- `(if ,len
- (bindat--pack-str ,len . ,args)
- (bindat--pack-strz . ,args))))))))
+ (`(pack . ,args) `(bindat--pack-strz ,len . ,args))))
(cl-defmethod bindat--type (op (_ (eql 'bits)) len)
(bindat--pcase op
--
2.36.1
[-- Attachment #1.1.3: v3-0002-bindat-strz-Null-terminate-fixed-length-strings-i.patch --]
[-- Type: text/x-patch, Size: 5298 bytes --]
From cf65b56e537fb7bab2b8776dfa876fa56f1839bc Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 16 Jun 2022 15:21:57 -0400
Subject: [PATCH v3 2/2] bindat (strz): Null terminate fixed-length strings if
there is room
* lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
fields, explicitly write a null terminator after the packed string if
there is room (bug#56048).
* doc/lispref/processes.texi (Bindat Types): Update documentation.
* test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
Update tests.
---
doc/lispref/processes.texi | 31 ++++++++++++++--------------
lisp/emacs-lisp/bindat.el | 13 ++++++------
test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
3 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index b9200aedde..9e0bd98a54 100644
--- a/doc/lispref/processes.texi
+++ b/doc/lispref/processes.texi
@@ -3509,23 +3509,24 @@ Bindat Types
(but excluding) the null byte that terminated the input string.
If @var{len} is provided, @code{strz} behaves the same as @code{str},
-but with one difference: when unpacking, the first null byte
-encountered in the packed string is interpreted as the terminating
-byte, and it and all subsequent bytes are excluded from the result of
-the unpacking.
+but with a couple of differences:
+
+@itemize @bullet
+@item
+When packing, a null terminator is written after the packed input
+string if the number of characters in the input string is less than
+@var{len}.
+
+@item
+When unpacking, the first null byte encountered in the packed string
+is interpreted as the terminating byte, and it and all subsequent
+bytes are excluded from the result of the unpacking.
+@end itemize
@quotation Caution
-The packed output will not be null-terminated unless one of the
-following is true:
-@itemize
-@item
-The input string is shorter than @var{len} bytes and either no pre-allocated
-string was provided to @code{bindat-pack} or the appropriate byte in
-the pre-allocated string was already null.
-@item
-The input string contains a null byte within the first @var{len}
-bytes.
-@end itemize
+The packed output will not be null-terminated unless the input string
+is shorter than @var{len} bytes or it contains a null byte within the
+first @var{len} bytes.
@end quotation
@item vec @var{len} [@var{type}]
diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 4a642bb9c5..0ecac3d52a 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -443,11 +443,14 @@ bindat--pack-str
(defun bindat--pack-strz (len v)
(let* ((v (string-to-unibyte v))
(vlen (length v)))
+ ;; Explicitly write a null terminator (if there's room) in case
+ ;; the user provided a pre-allocated string to `bindat-pack' that
+ ;; wasn't already zeroed.
+ (when (or (null len) (< vlen len))
+ (aset bindat-raw (+ bindat-idx vlen) 0))
(if len
;; When len is specified, behave the same as the str type
- ;; since we don't actually add the terminating zero anyway
- ;; (because we rely on the fact that `bindat-raw' was
- ;; presumably initialized with all-zeroes before we started).
+ ;; (except for the null terminator possibly written above).
(bindat--pack-str len v)
(dotimes (i vlen)
(when (= (aref v i) 0)
@@ -456,10 +459,6 @@ bindat--pack-strz
;; need to scan the input string looking for a null byte.
(error "Null byte encountered in input strz string"))
(aset bindat-raw (+ bindat-idx i) (aref v i)))
- ;; Explicitly write a null terminator in case the user provided
- ;; a pre-allocated string to `bindat-pack' that wasn't already
- ;; zeroed.
- (aset bindat-raw (+ bindat-idx vlen) 0)
(setq bindat-idx (+ bindat-idx vlen 1)))))
(defun bindat--pack-bits (len v)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index cc223ad14e..0c03c51e2e 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -172,14 +172,14 @@ bindat-test--str-strz-prealloc
((((x str 2)) ((x . "a"))) . "ax")
((((x str 2)) ((x . "ab"))) . "ab")
((((x str 2)) ((x . "abc"))) . "ab")
- ((,(bindat-type strz 1) "") . "xx")
- ((,(bindat-type strz 2) "") . "xx")
- ((,(bindat-type strz 2) "a") . "ax")
+ ((,(bindat-type strz 1) "") . "\0x")
+ ((,(bindat-type strz 2) "") . "\0x")
+ ((,(bindat-type strz 2) "a") . "a\0")
((,(bindat-type strz 2) "ab") . "ab")
((,(bindat-type strz 2) "abc") . "ab")
- ((((x strz 1)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . ""))) . "xx")
- ((((x strz 2)) ((x . "a"))) . "ax")
+ ((((x strz 1)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . ""))) . "\0x")
+ ((((x strz 2)) ((x . "a"))) . "a\0")
((((x strz 2)) ((x . "ab"))) . "ab")
((((x strz 2)) ((x . "abc"))) . "ab")
((,(bindat-type strz) "") . "\0x")
--
2.36.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
2022-06-18 3:02 ` Richard Hansen
2022-06-18 6:32 ` Eli Zaretskii
@ 2022-06-21 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-22 13:57 ` Eli Zaretskii
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-06-21 20:44 UTC (permalink / raw)
To: Richard Hansen; +Cc: 56048
Thanks, Richard. Looks good to me.
Stefan
Richard Hansen [2022-06-17 23:02:57] wrote:
> Attached are new revisions of the patches. The only differences are the
> comments were filled at column 70 instead of 80, and the commit message
> mentions the bug number.
>
> From 6096bc8bceada87a5c54e4eb500812a0b72ffd44 Mon Sep 17 00:00:00 2001
> From: Richard Hansen <rhansen@rhansen.org>
> Date: Sun, 29 May 2022 21:23:57 -0400
> Subject: [PATCH v2 1/2] ; bindat (strz): Move all pack logic to pack function
>
> ---
> lisp/emacs-lisp/bindat.el | 49 ++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 46e2a4901c..4a642bb9c5 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -440,20 +440,27 @@ bindat--pack-str
> (aset bindat-raw (+ bindat-idx i) (aref v i)))
> (setq bindat-idx (+ bindat-idx len))))
>
> -(defun bindat--pack-strz (v)
> +(defun bindat--pack-strz (len v)
> (let* ((v (string-to-unibyte v))
> - (len (length v)))
> - (dotimes (i len)
> - (when (= (aref v i) 0)
> - ;; Alternatively we could pretend that this was the end of
> - ;; the string and stop packing, but then bindat-length would
> - ;; need to scan the input string looking for a null byte.
> - (error "Null byte encountered in input strz string"))
> - (aset bindat-raw (+ bindat-idx i) (aref v i)))
> - ;; Explicitly write a null terminator in case the user provided a
> - ;; pre-allocated string to bindat-pack that wasn't zeroed first.
> - (aset bindat-raw (+ bindat-idx len) 0)
> - (setq bindat-idx (+ bindat-idx len 1))))
> + (vlen (length v)))
> + (if len
> + ;; When len is specified, behave the same as the str type
> + ;; since we don't actually add the terminating zero anyway
> + ;; (because we rely on the fact that `bindat-raw' was
> + ;; presumably initialized with all-zeroes before we started).
> + (bindat--pack-str len v)
> + (dotimes (i vlen)
> + (when (= (aref v i) 0)
> + ;; Alternatively we could pretend that this was the end of
> + ;; the string and stop packing, but then bindat-length would
> + ;; need to scan the input string looking for a null byte.
> + (error "Null byte encountered in input strz string"))
> + (aset bindat-raw (+ bindat-idx i) (aref v i)))
> + ;; Explicitly write a null terminator in case the user provided
> + ;; a pre-allocated string to `bindat-pack' that wasn't already
> + ;; zeroed.
> + (aset bindat-raw (+ bindat-idx vlen) 0)
> + (setq bindat-idx (+ bindat-idx vlen 1)))))
>
> (defun bindat--pack-bits (len v)
> (let ((bnum (1- (* 8 len))) j m)
> @@ -482,7 +489,8 @@ bindat--pack-item
> ('u24r (bindat--pack-u24r v))
> ('u32r (bindat--pack-u32r v))
> ('bits (bindat--pack-bits len v))
> - ((or 'str 'strz) (bindat--pack-str len v))
> + ('str (bindat--pack-str len v))
> + ('strz (bindat--pack-strz len v))
> ('vec
> (let ((l (length v)) (vlen 1))
> (if (consp vectype)
> @@ -699,18 +707,7 @@ bindat--type
> ((numberp len) len)
> ;; General expression support.
> (t `(or ,len (1+ (length ,val)))))))
> - (`(pack . ,args)
> - ;; When len is specified, behave the same as the str type since we don't
> - ;; actually add the terminating zero anyway (because we rely on the fact
> - ;; that `bindat-raw' was presumably initialized with all-zeroes before we
> - ;; started).
> - (cond ; Same optimizations as 'length above.
> - ((null len) `(bindat--pack-strz . ,args))
> - ((numberp len) `(bindat--pack-str ,len . ,args))
> - (t (macroexp-let2 nil len len
> - `(if ,len
> - (bindat--pack-str ,len . ,args)
> - (bindat--pack-strz . ,args))))))))
> + (`(pack . ,args) `(bindat--pack-strz ,len . ,args))))
>
> (cl-defmethod bindat--type (op (_ (eql 'bits)) len)
> (bindat--pcase op
> --
> 2.36.1
>
>
> From 9ebda68264adca6f60f780d44995c4213d2c12c2 Mon Sep 17 00:00:00 2001
> From: Richard Hansen <rhansen@rhansen.org>
> Date: Thu, 16 Jun 2022 15:21:57 -0400
> Subject: [PATCH v2 2/2] bindat (strz): Null terminate fixed-length strings if
> there is room
>
> * lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
> fields, explicitly write a null terminator after the packed string if
> there is room (bug#56048).
> * doc/lispref/processes.texi (Bindat Types): Update documentation.
> * test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
> Update tests.
> ---
> doc/lispref/processes.texi | 30 ++++++++++++++--------------
> lisp/emacs-lisp/bindat.el | 13 ++++++------
> test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
> 3 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
> index b9200aedde..d038d52d44 100644
> --- a/doc/lispref/processes.texi
> +++ b/doc/lispref/processes.texi
> @@ -3509,23 +3509,23 @@ Bindat Types
> (but excluding) the null byte that terminated the input string.
>
> If @var{len} is provided, @code{strz} behaves the same as @code{str},
> -but with one difference: when unpacking, the first null byte
> -encountered in the packed string is interpreted as the terminating
> -byte, and it and all subsequent bytes are excluded from the result of
> -the unpacking.
> +but with a couple of differences:
> +
> +@itemize @bullet
> +@item
> +When packing, a null terminator is written after the packed string if
> +the length of the input string is less than @var{len}.
> +
> +@item
> +When unpacking, the first null byte encountered in the packed string
> +is interpreted as the terminating byte, and it and all subsequent
> +bytes are excluded from the result of the unpacking.
> +@end itemize
>
> @quotation Caution
> -The packed output will not be null-terminated unless one of the
> -following is true:
> -@itemize
> -@item
> -The input string is shorter than @var{len} bytes and either no pre-allocated
> -string was provided to @code{bindat-pack} or the appropriate byte in
> -the pre-allocated string was already null.
> -@item
> -The input string contains a null byte within the first @var{len}
> -bytes.
> -@end itemize
> +The packed output will not be null-terminated unless the input string
> +is shorter than @var{len} bytes or it contains a null byte within the
> +first @var{len} bytes.
> @end quotation
>
> @item vec @var{len} [@var{type}]
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 4a642bb9c5..0ecac3d52a 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -443,11 +443,14 @@ bindat--pack-str
> (defun bindat--pack-strz (len v)
> (let* ((v (string-to-unibyte v))
> (vlen (length v)))
> + ;; Explicitly write a null terminator (if there's room) in case
> + ;; the user provided a pre-allocated string to `bindat-pack' that
> + ;; wasn't already zeroed.
> + (when (or (null len) (< vlen len))
> + (aset bindat-raw (+ bindat-idx vlen) 0))
> (if len
> ;; When len is specified, behave the same as the str type
> - ;; since we don't actually add the terminating zero anyway
> - ;; (because we rely on the fact that `bindat-raw' was
> - ;; presumably initialized with all-zeroes before we started).
> + ;; (except for the null terminator possibly written above).
> (bindat--pack-str len v)
> (dotimes (i vlen)
> (when (= (aref v i) 0)
> @@ -456,10 +459,6 @@ bindat--pack-strz
> ;; need to scan the input string looking for a null byte.
> (error "Null byte encountered in input strz string"))
> (aset bindat-raw (+ bindat-idx i) (aref v i)))
> - ;; Explicitly write a null terminator in case the user provided
> - ;; a pre-allocated string to `bindat-pack' that wasn't already
> - ;; zeroed.
> - (aset bindat-raw (+ bindat-idx vlen) 0)
> (setq bindat-idx (+ bindat-idx vlen 1)))))
>
> (defun bindat--pack-bits (len v)
> diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
> index cc223ad14e..0c03c51e2e 100644
> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -172,14 +172,14 @@ bindat-test--str-strz-prealloc
> ((((x str 2)) ((x . "a"))) . "ax")
> ((((x str 2)) ((x . "ab"))) . "ab")
> ((((x str 2)) ((x . "abc"))) . "ab")
> - ((,(bindat-type strz 1) "") . "xx")
> - ((,(bindat-type strz 2) "") . "xx")
> - ((,(bindat-type strz 2) "a") . "ax")
> + ((,(bindat-type strz 1) "") . "\0x")
> + ((,(bindat-type strz 2) "") . "\0x")
> + ((,(bindat-type strz 2) "a") . "a\0")
> ((,(bindat-type strz 2) "ab") . "ab")
> ((,(bindat-type strz 2) "abc") . "ab")
> - ((((x strz 1)) ((x . ""))) . "xx")
> - ((((x strz 2)) ((x . ""))) . "xx")
> - ((((x strz 2)) ((x . "a"))) . "ax")
> + ((((x strz 1)) ((x . ""))) . "\0x")
> + ((((x strz 2)) ((x . ""))) . "\0x")
> + ((((x strz 2)) ((x . "a"))) . "a\0")
> ((((x strz 2)) ((x . "ab"))) . "ab")
> ((((x strz 2)) ((x . "abc"))) . "ab")
> ((,(bindat-type strz) "") . "\0x")
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
2022-06-21 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-06-22 13:57 ` Eli Zaretskii
0 siblings, 0 replies; 6+ messages in thread
From: Eli Zaretskii @ 2022-06-22 13:57 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 56048-done, rhansen
> Cc: 56048@debbugs.gnu.org
> Date: Tue, 21 Jun 2022 16:44:06 -0400
> From: Stefan Monnier via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>
> Thanks, Richard. Looks good to me.
Installed on master.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-22 13:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-17 22:52 bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room Richard Hansen
2022-06-18 3:02 ` Richard Hansen
2022-06-18 6:32 ` Eli Zaretskii
2022-06-18 22:57 ` Richard Hansen
2022-06-21 20:44 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-06-22 13:57 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).