master 52e3ac6: Document and enforce some properties for strings created by modules.

[Philipp Stephani]

branch: master
commit 52e3ac6303292fdea8f441821a40f8f5ca31e3de
Author: Philipp Stephani <phst@google.com>
Commit: Philipp Stephani <phst@google.com>

    Document and enforce some properties for strings created by modules.
    
    When creating multibyte or unibyte strings, we should guarantee the
    following invariants:
    
    - When creating empty strings, a NULL data pointer should be allowed.
      This often arises in practice if the string length isn't known in
      advance, and we don't want to unnecessarily trigger undefined
      behavior.  Since functions like memcpy might not accept NULL
      pointers, use the canonical empty string objects in this case.
    
    - Nonzero strings should be guaranteed to be unique and mutable.
      These are the same guarantees expected from Lisp functions such as
      'make-string' or 'unibyte-string'.  On the other hand, empty strings
      might not be unique.
    
    * src/emacs-module.c (module_make_string)
    (module_make_unibyte_string): Correctly handle empty strings.
    
    * test/src/emacs-module-resources/mod-test.c (Fmod_test_make_string):
    New test function.
    (emacs_module_init): Expose it.
    
    * test/src/emacs-module-tests.el (mod-test-make-string/empty)
    (mod-test-make-string/nonempty): New unit tests.
    
    * doc/lispref/internals.texi (Module Values): Document properties and
    corner cases for strings.
---
 doc/lispref/internals.texi                 |  5 ++++-
 src/emacs-module.c                         |  8 ++++----
 test/src/emacs-module-resources/mod-test.c | 30 ++++++++++++++++++++++++++++++
 test/src/emacs-module-tests.el             | 20 ++++++++++++++++++++
 4 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index fb24544..28a5fdb 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1864,7 +1864,10 @@ byte, is @var{len}.  The original string in @var{str} 
can be either an
 it can include embedded null bytes, and doesn't have to end in a
 terminating null byte at @code{@var{str}[@var{len}]}.  The function
 raises the @code{overflow-error} error condition if @var{len} is
-negative or exceeds the maximum length of an Emacs string.
+negative or exceeds the maximum length of an Emacs string.  If
+@var{len} is zero, then @var{str} can be @code{NULL}, otherwise it
+must point to valid memory.  For nonzero @var{len}, @code{make_string}
+returns unique mutable string objects.
 @end deftypefn
 
 @deftypefn Function emacs_value make_unibyte_string (emacs_env *@var{env}, 
const char *@var{str}, ptrdiff_t @var{len})
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 0f3ef59..b7cd835 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -784,7 +784,8 @@ module_make_string (emacs_env *env, const char *str, 
ptrdiff_t len)
   MODULE_FUNCTION_BEGIN (NULL);
   if (! (0 <= len && len <= STRING_BYTES_BOUND))
     overflow_error ();
-  Lisp_Object lstr = module_decode_utf_8 (str, len);
+  Lisp_Object lstr
+    = len == 0 ? empty_multibyte_string : module_decode_utf_8 (str, len);
   return lisp_to_value (env, lstr);
 }
 
@@ -794,9 +795,8 @@ module_make_unibyte_string (emacs_env *env, const char 
*str, ptrdiff_t length)
   MODULE_FUNCTION_BEGIN (NULL);
   if (! (0 <= length && length <= STRING_BYTES_BOUND))
     overflow_error ();
-  Lisp_Object lstr = make_uninit_string (length);
-  memcpy (SDATA (lstr), str, length);
-  SDATA (lstr)[length] = 0;
+  Lisp_Object lstr
+    = length == 0 ? empty_unibyte_string : make_unibyte_string (str, length);
   return lisp_to_value (env, lstr);
 }
 
diff --git a/test/src/emacs-module-resources/mod-test.c 
b/test/src/emacs-module-resources/mod-test.c
index f855e9b..30ad352 100644
--- a/test/src/emacs-module-resources/mod-test.c
+++ b/test/src/emacs-module-resources/mod-test.c
@@ -24,6 +24,7 @@ along with GNU Emacs.  If not, see 
<https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
 #include <limits.h>
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -699,6 +700,34 @@ Fmod_test_funcall (emacs_env *env, ptrdiff_t nargs, 
emacs_value *args,
   return env->funcall (env, args[0], nargs - 1, args + 1);
 }
 
+static emacs_value
+Fmod_test_make_string (emacs_env *env, ptrdiff_t nargs,
+                       emacs_value *args, void *data)
+{
+  assert (nargs == 2);
+  intmax_t length_arg = env->extract_integer (env, args[0]);
+  if (env->non_local_exit_check (env) != emacs_funcall_exit_return)
+    return args[0];
+  if (length_arg < 0 || SIZE_MAX < length_arg)
+    {
+      signal_error (env, "Invalid string length");
+      return args[0];
+    }
+  size_t length = (size_t) length_arg;
+  bool multibyte = env->is_not_nil (env, args[1]);
+  char *buffer = length == 0 ? NULL : malloc (length);
+  if (buffer == NULL && length != 0)
+    {
+      memory_full (env);
+      return args[0];
+    }
+  memset (buffer, 'a', length);
+  emacs_value ret = multibyte ? env->make_string (env, buffer, length)
+                              : env->make_unibyte_string (env, buffer, length);
+  free (buffer);
+  return ret;
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -790,6 +819,7 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-async-pipe", Fmod_test_async_pipe, 1, 1, NULL, NULL);
   DEFUN ("mod-test-funcall", Fmod_test_funcall, 1, emacs_variadic_function,
          NULL, NULL);
+  DEFUN ("mod-test-make-string", Fmod_test_make_string, 2, 2, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 99d4caf..bf26ffb 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -30,6 +30,7 @@
 (require 'ert)
 (require 'ert-x)
 (require 'help-fns)
+(require 'subr-x)
 
 (defconst mod-test-emacs
   (expand-file-name invocation-name invocation-directory)
@@ -556,4 +557,23 @@ See Bug#36226."
     (thread-join thread-1)
     (thread-join thread-2)))
 
+(ert-deftest mod-test-make-string/empty ()
+  (dolist (multibyte '(nil t))
+    (ert-info ((format "Multibyte: %s" multibyte))
+      (let ((got (mod-test-make-string 0 multibyte)))
+        (should (stringp got))
+        (should (string-empty-p got))
+        (should (eq (multibyte-string-p got) multibyte))))))
+
+(ert-deftest mod-test-make-string/nonempty ()
+  (dolist (multibyte '(nil t))
+    (ert-info ((format "Multibyte: %s" multibyte))
+      (let ((first (mod-test-make-string 1 multibyte))
+            (second (mod-test-make-string 1 multibyte)))
+        (should (stringp first))
+        (should (eql (length first) 1))
+        (should (eq (multibyte-string-p first) multibyte))
+        (should (string-equal first second))
+        (should-not (eq first second))))))
+
 ;;; emacs-module-tests.el ends here