From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Federico Tedin Newsgroups: gmane.emacs.bugs Subject: bug#49723: 28.0.50; Test in coding.c for NUL bytes in filenames is not reliable Date: Wed, 15 Sep 2021 00:17:33 +0200 Message-ID: <87pmta6buq.fsf@gmail.com> References: <83o8ary5kl.fsf@gnu.org> <87pmtbj81v.fsf@gmail.com> <8335q7c655.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7860"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) Cc: phst@google.com, 49723@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Sep 15 00:18:19 2021 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 1mQGko-0001fi-OE for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 15 Sep 2021 00:18:14 +0200 Original-Received: from localhost ([::1]:39542 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mQGkn-0008Cg-30 for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 14 Sep 2021 18:18:13 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:43040) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mQGkc-0008CM-BH for bug-gnu-emacs@gnu.org; Tue, 14 Sep 2021 18:18:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:38796) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mQGkc-0004tC-3Q for bug-gnu-emacs@gnu.org; Tue, 14 Sep 2021 18:18:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mQGkb-0004NY-TX for bug-gnu-emacs@gnu.org; Tue, 14 Sep 2021 18:18:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Federico Tedin Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 14 Sep 2021 22:18:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 49723 X-GNU-PR-Package: emacs Original-Received: via spool by 49723-submit@debbugs.gnu.org id=B49723.163165786516811 (code B ref 49723); Tue, 14 Sep 2021 22:18:01 +0000 Original-Received: (at 49723) by debbugs.gnu.org; 14 Sep 2021 22:17:45 +0000 Original-Received: from localhost ([127.0.0.1]:50342 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mQGkL-0004N4-CX for submit@debbugs.gnu.org; Tue, 14 Sep 2021 18:17:45 -0400 Original-Received: from mail-wr1-f47.google.com ([209.85.221.47]:39897) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mQGkG-0004Mo-SF for 49723@debbugs.gnu.org; Tue, 14 Sep 2021 18:17:44 -0400 Original-Received: by mail-wr1-f47.google.com with SMTP id u15so629930wru.6 for <49723@debbugs.gnu.org>; Tue, 14 Sep 2021 15:17:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=qeni89jRY1r4tzI516tbSKXQONFdIy1IM6dXfzgkjDo=; b=Wya4TW6FNVT3K8AG2cJipMcQgimX3m2R+dI+6GiS9RP1MN+DFtTSLbK/tlQ7DBmdeC WNODY+RKNo5rQZfVqBgTBbbPOUC7ZMedRKuyTPjr5XjH7P0zEsJXWQ8SFtZS1ZikeELH 2+Lwe5EWFivpKcLcYEpL17pc5nugcRSXzRPBvtpINajWcu2z7gtP5q6oX/0bfP5vI3P5 NmLbl0E9/cWnYwP2ycWJmt6tjc4HQhJMrC9Z6bD2+PaaDR2Cg5yDZpHBADliJAC+RplT asYKIWE4KtmPU9p8ot2+YxSkoD/+Ex/dP2vYXkfijelJ9xwmkVPlZW5zGOkFNNOMBZou OR/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=qeni89jRY1r4tzI516tbSKXQONFdIy1IM6dXfzgkjDo=; b=WQodAaREdj3SBRUQOe1hNlYC2svhpj37eEF49VwFSTjf9jIv6vmoH+PgP2xBuOA2bf iMwykojbDGw4NEQ6Xkh9mQLmjRXCaq+JZr8kakSbI5qxEoSFBUiLNSAPj+K92pXDd0ny G0Y0T3QbehbF+YNJ38BUWh7yPqfH1JXdC4IdN3/giPXpN9hNaXkCnLZUnHB7Zk6kUKRE fuCU9u3aN3iafRU5JCHgE2O1/SPGMoDyj7KwFzHOkOKSNJS6uMfnGVqzHlRgTP4fpjUd tEAzO0tGNf5K9eqD/tE/bi8HsDomfzOxZwEC5uOstHs+kMIQiVga0lPdems1024xCYGb 4+og== X-Gm-Message-State: AOAM532Se8Kyx1lNu4H51El+rlzk2fYD6dUYfCFTpbVQamyUgIHymOKV jRKKpIAf+9/3h/upfi2xdIrS/IFcqWI= X-Google-Smtp-Source: ABdhPJxtyqo5c9eZgx12+GuGGPJKiFri8TfyT3VnwqjeEq2bCmEo0rBN9zEGdMzInDS/+8XA840Fvw== X-Received: by 2002:a5d:46cb:: with SMTP id g11mr1472347wrs.60.1631657854675; Tue, 14 Sep 2021 15:17:34 -0700 (PDT) Original-Received: from gehirn (ip5b4202e5.dynamic.kabel-deutschland.de. [91.66.2.229]) by smtp.gmail.com with ESMTPSA id t1sm2505012wrz.39.2021.09.14.15.17.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Sep 2021 15:17:34 -0700 (PDT) In-Reply-To: <8335q7c655.fsf@gnu.org> (Eli Zaretskii's message of "Tue, 14 Sep 2021 22:24:22 +0300") 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:214343 Archived-At: --=-=-= Content-Type: text/plain Ok! Here's my first try at this. I ended up skipping the check on DEFAULT-DIRECTORY since as you mentioned, its value is used with expand-file-name itself. In the other case, if default-directory is picked up, then I checked the value of that variable. --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=expand-file-name.patch Content-Description: patch >From 0f960aeab1c4b3182d597ac0ce80526f09e97452 Mon Sep 17 00:00:00 2001 From: Federico Tedin Date: Wed, 15 Sep 2021 00:15:16 +0200 Subject: [PATCH] Add null byte checks to filenames in expand-file-name (bug#49723) * src/fileio.c (expand-file-name): Check for null bytes for both NAME and DEFAULT-DIRECTORY arguments. Also check for null bytes in buffer-local default-directory, assuming it is used. * src/coding.c (encode_file_name): Use CHECK_STRING_NULL_BYTES. * src/lisp.h (CHECK_STRING_NULL_BYTES): Add function for checking for null bytes in Lisp strings. * test/src/fileio-tests.el (fileio-test--expand-file-name-null-bytes): Add test for new changes to expand-file-name. * etc/NEWS: Announce changes. --- etc/NEWS | 6 ++++++ src/coding.c | 3 +-- src/fileio.c | 6 +++++- src/lisp.h | 7 +++++++ test/src/fileio-tests.el | 9 +++++++++ 5 files changed, 28 insertions(+), 3 deletions(-) diff --git a/etc/NEWS b/etc/NEWS index 5809716868..8d96ceff79 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -278,6 +278,12 @@ personalize the uniquified buffer name. --- ** 'remove-hook' is now an interactive command. +** 'expand-file-name' now checks for null bytes in filenames. +The function will now check for null bytes in both NAME and +DEFAULT-DIRECTORY arguments, as well as in the 'default-directory' +buffer-local variable, assuming its value is used. + +--- ** Frames +++ diff --git a/src/coding.c b/src/coding.c index d027c7d539..7030a53869 100644 --- a/src/coding.c +++ b/src/coding.c @@ -10430,8 +10430,7 @@ encode_file_name (Lisp_Object fname) cause subtle bugs because the system would silently use a different filename than expected. Perform this check after encoding to not miss NUL bytes introduced through encoding. */ - CHECK_TYPE (memchr (SSDATA (encoded), '\0', SBYTES (encoded)) == NULL, - Qfilenamep, fname); + CHECK_STRING_NULL_BYTES (encoded); return encoded; } diff --git a/src/fileio.c b/src/fileio.c index 0db8ed793b..3c13d3fe41 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -945,6 +945,7 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0, USE_SAFE_ALLOCA; CHECK_STRING (name); + CHECK_STRING_NULL_BYTES (name); /* If the file name has special constructs in it, call the corresponding file name handler. */ @@ -993,7 +994,10 @@ DEFUN ("expand-file-name", Fexpand_file_name, Sexpand_file_name, 1, 2, 0, if (STRINGP (dir)) { if (file_name_absolute_no_tilde_p (dir)) - default_directory = dir; + { + CHECK_STRING_NULL_BYTES (dir); + default_directory = dir; + } else { Lisp_Object absdir diff --git a/src/lisp.h b/src/lisp.h index 7bfc69b647..9716b34bae 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -1615,6 +1615,13 @@ STRING_SET_CHARS (Lisp_Object string, ptrdiff_t newsize) XSTRING (string)->u.s.size = newsize; } +INLINE void +CHECK_STRING_NULL_BYTES (Lisp_Object string) +{ + CHECK_TYPE (memchr (SSDATA (string), '\0', SBYTES (string)) == NULL, + Qfilenamep, string); +} + /* A regular vector is just a header plus an array of Lisp_Objects. */ struct Lisp_Vector diff --git a/test/src/fileio-tests.el b/test/src/fileio-tests.el index f4d123b426..438ebebb76 100644 --- a/test/src/fileio-tests.el +++ b/test/src/fileio-tests.el @@ -136,6 +136,15 @@ fileio-tests--relative-default-directory (should (and (file-name-absolute-p name) (not (eq (aref name 0) ?~)))))) +(ert-deftest fileio-test--expand-file-name-null-bytes () + "Test that expand-file-name checks for null bytes in filenames." + (should-error (expand-file-name (concat "file" (char-to-string ?\0) ".txt")) + :type 'wrong-type-argument) + (should-error (expand-file-name "file.txt" (concat "dir" (char-to-string ?\0))) + :type 'wrong-type-argument) + (let ((default-directory (concat "dir" (char-to-string ?\0)))) + (should-error (expand-file-name "file.txt") :type 'wrong-type-argument))) + (ert-deftest fileio-tests--file-name-absolute-p () "Test file-name-absolute-p." (dolist (suffix '("" "/" "//" "/foo" "/foo/" "/foo//" "/foo/bar")) -- 2.25.1 --=-=-= Content-Type: text/plain Eli Zaretskii writes: >> From: Federico Tedin >> Cc: 49723@debbugs.gnu.org, Philipp Stephani >> Date: Tue, 14 Sep 2021 21:01:16 +0200 >> >> I'm interested in looking into this one since I want to learn more about >> the C side of the codebase. However, I wasn't able to find a call to >> expand-file-name in encode_file_name or encode_file_name_1. I did find >> the null byte check though (CHECK_TYPE + memchr). Maybe I am missing >> something out. > > My description was inaccurate: the expand-file-name call usually > precedes the call to ENCODE_FILE, it is not part of encode_file_name. > >> I assume that a similar check on expand-file-name should be applied to >> both input arguments, NAME and DEFAULT-DIRECTORY? > > I don't think we need that because expand-file-name calls itself on > DEFAULT-DIRECTORY internally. But we may need to perform the check on > default-directory, if we use it inside expand-file-name. --=-=-=--