From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.devel Subject: [PATCH] Module assertions: check for garbage collections Date: Tue, 4 Jul 2017 22:55:08 +0200 Message-ID: <20170704205508.52067-1-phst@google.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1499201737 7463 195.159.176.226 (4 Jul 2017 20:55:37 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 4 Jul 2017 20:55:37 +0000 (UTC) Cc: Philipp Stephani To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Tue Jul 04 22:55:30 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dSUr3-0001T2-Lc for ged-emacs-devel@m.gmane.org; Tue, 04 Jul 2017 22:55:29 +0200 Original-Received: from localhost ([::1]:42925 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dSUr6-0004lT-1V for ged-emacs-devel@m.gmane.org; Tue, 04 Jul 2017 16:55:32 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:33789) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dSUqy-0004lI-9e for emacs-devel@gnu.org; Tue, 04 Jul 2017 16:55:25 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dSUqv-0000Of-5k for emacs-devel@gnu.org; Tue, 04 Jul 2017 16:55:24 -0400 Original-Received: from mail-wr0-x232.google.com ([2a00:1450:400c:c0c::232]:33652) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dSUqu-0000OD-RZ for emacs-devel@gnu.org; Tue, 04 Jul 2017 16:55:21 -0400 Original-Received: by mail-wr0-x232.google.com with SMTP id r103so254045141wrb.0 for ; Tue, 04 Jul 2017 13:55:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=4P6h19O5AlojyYs0ACIapIL+amH6bAR25Dz8b8iYPKA=; b=Hv/U58PteZYGgYr5Yjtw8xhXl4EMsr/8uv+L+L/I5c4iO3qKghpizdRxhhJWlxAV4C rFbmO9Zpg9EzVtKKMI0qJ+5YYXB8gyx3whHCWvWQeK8+oae1fjFz29rj9VdJPcBnI+jk rJfR9Zoxdfct39r6qnqNc2wuiuzgThlpyVc2nJF4lBKmEjRxnv29vkhXJEyntDq1f4ML yIUSQajDF5x50j0AYhpkjRLMpMu+EpvSLueSXAEkwti179ersFpveGw41Jke8w8RHhyB URt6u1hMRcNRmPYtS9JzOoHig2dubhPG3N3t+hCSlYhvG//ZcYjpmtXLxZ30FUa81a1y W53Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=4P6h19O5AlojyYs0ACIapIL+amH6bAR25Dz8b8iYPKA=; b=NyuK+Zb06Lvw/qEYndseS1oM93U8ryHzA4Exn2ThL74iK9w73VIlpjpdLnvTiDLk0y 9I3Olww1IkEK5g4VJxrLWOiMUWVbj5UOhocAxehy/O9UQ4o/STrkUAi17gnfkvJmZxTW LYyOUQLG5wKhiiCSfoWwSmCJWogylULlr2qF8LOydxJ/baN2tTDrU5AWtYwngWRm4wUb LPvwhfuSbpypMTi45LYjjQdoltpiS8I2hB8k3ma1lGaFRpams4Nr73fxLTpRx7R+6hj+ JftvXaWtYGZ9+00Wck+ZNW42ax59gDyTyxEeg6/AOUxicsZIL9t6s2iluokL66r8zzNc FQ5Q== X-Gm-Message-State: AKS2vOzE5hXVgv28bniW+20syXjdFk7xSabKz3UgPCVEswM2lZmJ1/qG j92xTjuAgaSaOFMPNE0= X-Received: by 10.223.172.67 with SMTP id v61mr41369377wrc.112.1499201717797; Tue, 04 Jul 2017 13:55:17 -0700 (PDT) Original-Received: from p.cm.cablesurf.de (46.128.198.151.dynamic.cablesurf.de. [46.128.198.151]) by smtp.gmail.com with ESMTPSA id z101sm29686738wrb.41.2017.07.04.13.55.16 (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 04 Jul 2017 13:55:17 -0700 (PDT) X-Google-Original-From: Philipp Stephani X-Mailer: git-send-email 2.13.2 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c0c::232 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:216174 Archived-At: It's technically possible to write a user pointer finalizer that calls into Emacs module functions. This would be disastrous because it would allow arbitrary Lisp code to run during garbage collection. Therefore extend the module assertions to check for this case. * src/emacs-module.c (module_assert_thread): Also check whether a garbage collection is in progress. * test/data/emacs-module/mod-test.c (invalid_finalizer) (Fmod_test_invalid_finalizer): New test module functions. (emacs_module_init): Register new test function. * test/src/emacs-module-tests.el (module--test-assertion) (module--with-temp-directory): New helper macros. (module--test-assertions--load-non-live-object): Rename existing unit test, use helper macros. (module--test-assertions--call-emacs-from-gc): New unit test. --- src/emacs-module.c | 6 ++- test/data/emacs-module/mod-test.c | 23 +++++++++++ test/src/emacs-module-tests.el | 87 ++++++++++++++++++++++++++------------- 3 files changed, 85 insertions(+), 31 deletions(-) diff --git a/src/emacs-module.c b/src/emacs-module.c index 7b1a402eef..b80aa23abc 100644 --- a/src/emacs-module.c +++ b/src/emacs-module.c @@ -817,9 +817,11 @@ in_current_thread (void) static void module_assert_thread (void) { - if (! module_assertions || in_current_thread ()) + if (! module_assertions || (in_current_thread () && ! gc_in_progress)) return; - module_abort ("Module function called from outside the current Lisp thread"); + module_abort (gc_in_progress ? + "Module function called during garbage collection" : + "Module function called from outside the current Lisp thread"); } static void diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c index eee9466c5d..42e1c2bd4a 100644 --- a/test/data/emacs-module/mod-test.c +++ b/test/data/emacs-module/mod-test.c @@ -235,6 +235,27 @@ Fmod_test_invalid_load (emacs_env *env, ptrdiff_t nargs, emacs_value *args, return invalid_stored_value; } +/* An invalid finalizer: Finalizers are run during garbage collection, + where Lisp code can’t be executed. -module-assertions tests for + this case. */ + +static emacs_env *current_env; + +static void +invalid_finalizer (void *ptr) +{ + current_env->intern (current_env, "nil"); +} + +static emacs_value +Fmod_test_invalid_finalizer (emacs_env *env, ptrdiff_t nargs, emacs_value *args, + void *data) +{ + current_env = env; + env->make_user_ptr (env, invalid_finalizer, NULL); + return env->funcall (env, env->intern (env, "garbage-collect"), 0, NULL); +} + /* Lisp utilities for easier readability (simple wrappers). */ @@ -300,6 +321,8 @@ emacs_module_init (struct emacs_runtime *ert) DEFUN ("mod-test-vector-eq", Fmod_test_vector_eq, 2, 2, NULL, NULL); DEFUN ("mod-test-invalid-store", Fmod_test_invalid_store, 0, 0, NULL, NULL); DEFUN ("mod-test-invalid-load", Fmod_test_invalid_load, 0, 0, NULL, NULL); + DEFUN ("mod-test-invalid-finalizer", Fmod_test_invalid_finalizer, 0, 0, + NULL, NULL); #undef DEFUN diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el index a4994b6223..988a7a178c 100644 --- a/test/src/emacs-module-tests.el +++ b/test/src/emacs-module-tests.el @@ -182,37 +182,66 @@ multiply-string (should (equal (help-function-arglist #'mod-test-sum) '(arg1 arg2)))) -(ert-deftest module--test-assertions () - "Check that -module-assertions work." +(defmacro module--with-temp-directory (name &rest body) + "Bind NAME to the name of a temporary directory and evaluate BODY. +NAME must be a symbol. Delete the temporary directory after BODY +exits normally or non-locally. NAME will be bound to the +directory name (not the directory file name) of the temporary +directory." + (declare (indent 1)) + (cl-check-type name symbol) + `(let ((,name (file-name-as-directory + (make-temp-file "emacs-module-test" :directory)))) + (unwind-protect + (progn ,@body) + (delete-directory ,name :recursive)))) + +(defmacro module--test-assertion (pattern &rest body) + "Test that PATTERN matches the assertion triggered by BODY. +Run Emacs as a subprocess, load the test module `mod-test-file', +and evaluate BODY. Verify that Emacs aborts and prints a module +assertion message that matches PATTERN. PATTERN is evaluated and +must evaluate to a regular expression string." + (declare (indent 1)) + ;; To contain any core dumps. + `(module--with-temp-directory tempdir + (with-temp-buffer + (let* ((default-directory tempdir) + (status (call-process mod-test-emacs nil t nil + "-batch" "-Q" "-module-assertions" "-eval" + ,(prin1-to-string + `(progn + (require 'mod-test ,mod-test-file) + ,@body))))) + (should (stringp status)) + ;; eg "Aborted" or "Abort trap: 6" + (should (string-prefix-p "Abort" status)) + (search-backward "Emacs module assertion: ") + (goto-char (match-end 0)) + (should (string-match-p ,pattern + (buffer-substring-no-properties + (point) (point-max)))))))) + +(ert-deftest module--test-assertions--load-non-live-object () + "Check that -module-assertions verify that non-live objects +aren’t accessed." (skip-unless (file-executable-p mod-test-emacs)) ;; This doesn’t yet cause undefined behavior. (should (eq (mod-test-invalid-store) 123)) - ;; To contain any core dumps. - (let ((tempdir (make-temp-file "emacs-module-test" t))) - (unwind-protect - (with-temp-buffer - (should (string-match-p - "Abort" ; eg "Aborted" or "Abort trap: 6" - (let ((default-directory tempdir)) - (call-process mod-test-emacs nil t nil - "-batch" "-Q" "-module-assertions" "-eval" - (prin1-to-string - `(progn - (require 'mod-test ,mod-test-file) - ;; Storing and reloading a local - ;; value causes undefined behavior, - ;; which should be detected by the - ;; module assertions. - (mod-test-invalid-store) - (mod-test-invalid-load))))))) - (search-backward "Emacs module assertion:") - (should (string-match-p (rx bos "Emacs module assertion: " - "Emacs value not found in " - (+ digit) " values of " - (+ digit) " environments" eos) - (buffer-substring-no-properties - (line-beginning-position) - (line-end-position))))) - (delete-directory tempdir t)))) + (module--test-assertion (rx "Emacs value not found in " + (+ digit) " values of " + (+ digit) " environments\n" eos) + ;; Storing and reloading a local value causes undefined behavior, + ;; which should be detected by the module assertions. + (mod-test-invalid-store) + (mod-test-invalid-load))) + +(ert-deftest module--test-assertions--call-emacs-from-gc () + "Check that -module-assertions prevents calling Emacs functions +during garbage collection." + (skip-unless (file-executable-p mod-test-emacs)) + (module--test-assertion + (rx "Module function called during garbage collection\n" eos) + (mod-test-invalid-finalizer))) ;;; emacs-module-tests.el ends here -- 2.13.2