From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room Date: Tue, 21 Jun 2022 16:44:06 -0400 Message-ID: References: <8b471c36-abbe-819c-96d8-8f0d7b671afb@rhansen.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7781"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: 56048@debbugs.gnu.org To: Richard Hansen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Jun 21 22:45:30 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1o3kkb-0001hj-9u for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 21 Jun 2022 22:45:29 +0200 Original-Received: from localhost ([::1]:45298 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o3kkZ-0004zz-P7 for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 21 Jun 2022 16:45:27 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:53080) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o3kkA-0004zl-Uc for bug-gnu-emacs@gnu.org; Tue, 21 Jun 2022 16:45:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:39538) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1o3kkA-0000ac-IE for bug-gnu-emacs@gnu.org; Tue, 21 Jun 2022 16:45:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1o3kkA-0004ln-GL for bug-gnu-emacs@gnu.org; Tue, 21 Jun 2022 16:45:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 21 Jun 2022 20:45:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 56048 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 56048-submit@debbugs.gnu.org id=B56048.165584425918261 (code B ref 56048); Tue, 21 Jun 2022 20:45:02 +0000 Original-Received: (at 56048) by debbugs.gnu.org; 21 Jun 2022 20:44:19 +0000 Original-Received: from localhost ([127.0.0.1]:33435 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1o3kjS-0004kS-UE for submit@debbugs.gnu.org; Tue, 21 Jun 2022 16:44:19 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:57749) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1o3kjR-0004kD-9s for 56048@debbugs.gnu.org; Tue, 21 Jun 2022 16:44:17 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id CAEAB441828; Tue, 21 Jun 2022 16:44:11 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id F05A744181B; Tue, 21 Jun 2022 16:44:08 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1655844248; bh=PAG5fK/1yHBnJhrL0j3h3+ivlYcbWIaX2DBddgHb2sQ=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=aXRBvm0nKN2wNLwAt/pwSVRBKCR/2ELwphAZVUx976nuDBLoqyagCe6V9uW+OYSS4 1fP2roVJtxKytZIR3OcVseLE9fO20/DomZ1kPtsFbRWsFCiGT3KO0vK5A0aWqdfSs9 N+DNJdkMWdpHfEtROo/4vKHm4ukj8lHwoivyiGjphzjOatDirnXnb6O5SWsL+xJnVL 2Y1Hi/q7F2Sj+IPGbTiNFRBDwePqdJimpDqjmMIm3iLztg2zmzHLFE45cBRhZN/pd4 LMeeY33CuD7A8kLwHWZLgdl+zPb1/23CItR6dvMxF+CzwWNWpwZRdzenrxfiRe3BDD 1O27nR/pJTcpA== Original-Received: from alfajor (unknown [80.82.234.185]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 5D2DF120403; Tue, 21 Jun 2022 16:44:08 -0400 (EDT) In-Reply-To: (Richard Hansen's message of "Fri, 17 Jun 2022 23:02:57 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:234983 Archived-At: 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 > 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 > 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")