bug#44411: 28.0.50; uudecode-decode-region-internal is broken

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Kazuhiro Ito]

When I call uudecode-decode-region-internal in multibyte buffer, it
fails to decode eight-bit characters.

The function makes string from uuencoded text by passing unsigned char
vlue (0-255) to char-to-string function, which makes multibyte-string.
After that, string is decoded as binary.  But eight-bit characters are
never made in that way.

(let ((ch #xc8))
  (decode-coding-string (char-to-string ch) 'binary))

-> "8"

Additionally, concat and char-to-string functions are called so
frequently that deocder is very slow for large data.

Please see the below patch.


diff --git a/lisp/mail/uudecode.el b/lisp/mail/uudecode.el
index bcbd571b53..f9254aee75 100644
--- a/lisp/mail/uudecode.el
+++ b/lisp/mail/uudecode.el
@@ -149,12 +149,10 @@ uudecode-decode-region-internal
 	      (setq counter (1+ counter)
 		    inputpos (1+ inputpos))
 	      (cond ((= counter 4)
-		     (setq result (cons
-				   (concat
-				    (char-to-string (ash bits -16))
-				    (char-to-string (logand (ash bits -8) 255))
-				    (char-to-string (logand bits 255)))
-				   result))
+		     (setq result (cons (logand bits 255)
+					(cons (logand (ash bits -8) 255)
+					      (cons (ash bits -16)
+						    result))))
 		     (setq bits 0 counter 0))
 		    (t (setq bits (ash bits 6)))))))
 	  (cond
@@ -166,26 +164,21 @@ uudecode-decode-region-internal
 	    ;;(error "uucode ends unexpectedly")
 	    (setq done t))
 	   ((= counter 3)
-	    (setq result (cons
-			  (concat
-			   (char-to-string (logand (ash bits -16) 255))
-			   (char-to-string (logand (ash bits -8) 255)))
-			  result)))
+	    (setq result (cons (logand (ash bits -8) 255)
+			       (cons (logand (ash bits -16) 255)
+				     result))))
 	   ((= counter 2)
-	    (setq result (cons
-			  (char-to-string (logand (ash bits -10) 255))
-			  result))))
+	    (setq result (cons (logand (ash bits -10) 255)
+			       result))))
 	  (skip-chars-forward non-data-chars end))
+	(setq result (apply #'unibyte-string (nreverse result)))
 	(if file-name
             (with-temp-file file-name
               (set-buffer-multibyte nil)
-              (insert (apply #'concat (nreverse result))))
+              (insert result))
 	  (or (markerp end) (setq end (set-marker (make-marker) end)))
 	  (goto-char start)
-	  (if enable-multibyte-characters
-	      (dolist (x (nreverse result))
-                (insert (decode-coding-string x 'binary)))
-	    (insert (apply #'concat (nreverse result))))
+          (insert result)
 	  (delete-region (point) end))))))
 
 ;;;###autoload

-- 
Kazuhiro Ito

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Lars Ingebrigtsen]

Kazuhiro Ito <kzhr@d1.dion.ne.jp> writes:

> The function makes string from uuencoded text by passing unsigned char
> vlue (0-255) to char-to-string function, which makes multibyte-string.
> After that, string is decoded as binary.  But eight-bit characters are
> never made in that way.
>
> (let ((ch #xc8))
>   (decode-coding-string (char-to-string ch) 'binary))
>
> -> "8"
>
> Additionally, concat and char-to-string functions are called so
> frequently that deocder is very slow for large data.
>
> Please see the below patch.

Thanks; looks good to me.  This patch is slightly too large to apply
without having an FSF copyright on file, and I don't see that in the
assignment file for you.

Would you be willing to sign such paperwork so that we can get the patch
applied?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Eli Zaretskii]

> Date: Tue, 03 Nov 2020 17:27:41 +0900
> From: Kazuhiro Ito <kzhr@d1.dion.ne.jp>
> 
> When I call uudecode-decode-region-internal in multibyte buffer, it
> fails to decode eight-bit characters.

What do you mean by "decode eight-bit characters"?  There's no such
thing in the "real world", it is entirely an Emacs invention.  How
could such "characters" appear in uuencoded email message?

Can you describe the real-life use case where this happened?

> Additionally, concat and char-to-string functions are called so
> frequently that deocder is very slow for large data.

Please show only the changes to fix what you think is a bug.  let's
leave the optimization alone for a moment, so that it doesn't muddy
the waters.

Thanks.

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Kazuhiro Ito]

> > When I call uudecode-decode-region-internal in multibyte buffer, it
> > fails to decode eight-bit characters.
> 
> What do you mean by "decode eight-bit characters"?

I mean "decode uuencoded raw bytes 128-255 as eight-bit characters".

> How could such "characters" appear in uuencoded email message?

I did not said anything about uuencoded email message.  What do you
mean?

> Can you describe the real-life use case where this happened?

1. Yank below uuencoded string into multibyte buffer.

begin 644 c8c8c8c8.bin
$R,C(R```@
``
end
size 4

2. C-SPC at the beginning of uuencoded text and move point to the end
of uuencoded text.

3. M-x uudecode-decode-region-internal

4. decoded result is broken.  Original data is 4bytes of 0xc8, but
inserted text is "8888".  uudecode-decode-region-external returns
expected result.

> Please show only the changes to fix what you think is a bug.

Here is.

diff --git a/lisp/mail/uudecode.el b/lisp/mail/uudecode.el
index bcbd571b53..81a0a8ae0d 100644
--- a/lisp/mail/uudecode.el
+++ b/lisp/mail/uudecode.el
@@ -151,9 +151,9 @@ uudecode-decode-region-internal
 	      (cond ((= counter 4)
 		     (setq result (cons
 				   (concat
-				    (char-to-string (ash bits -16))
-				    (char-to-string (logand (ash bits -8) 255))
-				    (char-to-string (logand bits 255)))
+				    (unibyte-string (ash bits -16))
+				    (unibyte-string (logand (ash bits -8) 255))
+				    (unibyte-string (logand bits 255)))
 				   result))
 		     (setq bits 0 counter 0))
 		    (t (setq bits (ash bits 6)))))))
@@ -168,12 +168,12 @@ uudecode-decode-region-internal
 	   ((= counter 3)
 	    (setq result (cons
 			  (concat
-			   (char-to-string (logand (ash bits -16) 255))
-			   (char-to-string (logand (ash bits -8) 255)))
+			   (unibyte-string (logand (ash bits -16) 255))
+			   (unibyte-string (logand (ash bits -8) 255)))
 			  result)))
 	   ((= counter 2)
 	    (setq result (cons
-			  (char-to-string (logand (ash bits -10) 255))
+			  (unibyte-string (logand (ash bits -10) 255))
 			  result))))
 	  (skip-chars-forward non-data-chars end))
 	(if file-name
@@ -184,7 +184,7 @@ uudecode-decode-region-internal
 	  (goto-char start)
 	  (if enable-multibyte-characters
 	      (dolist (x (nreverse result))
-                (insert (decode-coding-string x 'binary)))
+                (insert x))
 	    (insert (apply #'concat (nreverse result))))
 	  (delete-region (point) end))))))
 

-- 
Kazuhiro Ito

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Eli Zaretskii]

> Date: Wed, 04 Nov 2020 17:26:56 +0900
> From: Kazuhiro Ito <kzhr@d1.dion.ne.jp>
> Cc: 44411@debbugs.gnu.org
> 
> > What do you mean by "decode eight-bit characters"?
> 
> I mean "decode uuencoded raw bytes 128-255 as eight-bit characters".
> 
> > How could such "characters" appear in uuencoded email message?
> 
> I did not said anything about uuencoded email message.  What do you
> mean?

Sorry, it looks like I misunderstood the use case.  Ignore that
question.

> 1. Yank below uuencoded string into multibyte buffer.
> 
> begin 644 c8c8c8c8.bin
> $R,C(R```@
> ``
> end
> size 4
> 
> 2. C-SPC at the beginning of uuencoded text and move point to the end
> of uuencoded text.
> 
> 3. M-x uudecode-decode-region-internal
> 
> 4. decoded result is broken.  Original data is 4bytes of 0xc8, but
> inserted text is "8888".  uudecode-decode-region-external returns
> expected result.

OK, I see the problem now: it's the call to decode-coding-string,
which replaced string-as-unibyte of yore.

> > Please show only the changes to fix what you think is a bug.
> 
> Here is.

OK, I agree also to your other optimizations, but can we please go a
step further and avoid consing a string here?  'insert' is perfectly
capable of inserting characters, not only strings.  So in the
unibyte-buffer case, just something like

  (apply #'insert (nreverse result))

with 'result' being a list of bytes, will produce what you want.  And
in the multibyte-buffer case you just need to convert each byte to its
Unicode-compatible codepoint, by using

  (decode-char 'eight-bit CH)

probably in 'mapcar' or somesuch, and then call 'insert'.

Can you augment your patch along these lines, please?

Thanks.

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Kazuhiro Ito]

> OK, I agree also to your other optimizations, but can we please go a
> step further and avoid consing a string here?  'insert' is perfectly
> capable of inserting characters, not only strings.  So in the
> unibyte-buffer case, just something like
> 
>   (apply #'insert (nreverse result))
> 
> with 'result' being a list of bytes, will produce what you want.  And
> in the multibyte-buffer case you just need to convert each byte to its
> Unicode-compatible codepoint, by using
> 
>   (decode-char 'eight-bit CH)
> 
> probably in 'mapcar' or somesuch, and then call 'insert'.
> 
> Can you augment your patch along these lines, please?

Here is a revised one.


diff --git a/lisp/mail/uudecode.el b/lisp/mail/uudecode.el
index bcbd571b53..0dce9b7b72 100644
--- a/lisp/mail/uudecode.el
+++ b/lisp/mail/uudecode.el
@@ -149,12 +149,10 @@ uudecode-decode-region-internal
 	      (setq counter (1+ counter)
 		    inputpos (1+ inputpos))
 	      (cond ((= counter 4)
-		     (setq result (cons
-				   (concat
-				    (char-to-string (ash bits -16))
-				    (char-to-string (logand (ash bits -8) 255))
-				    (char-to-string (logand bits 255)))
-				   result))
+		     (setq result (cons (logand bits 255)
+					(cons (logand (ash bits -8) 255)
+					      (cons (ash bits -16)
+						    result))))
 		     (setq bits 0 counter 0))
 		    (t (setq bits (ash bits 6)))))))
 	  (cond
@@ -166,26 +164,26 @@ uudecode-decode-region-internal
 	    ;;(error "uucode ends unexpectedly")
 	    (setq done t))
 	   ((= counter 3)
-	    (setq result (cons
-			  (concat
-			   (char-to-string (logand (ash bits -16) 255))
-			   (char-to-string (logand (ash bits -8) 255)))
-			  result)))
+	    (setq result (cons (logand (ash bits -8) 255)
+			       (cons (logand (ash bits -16) 255)
+				     result))))
 	   ((= counter 2)
-	    (setq result (cons
-			  (char-to-string (logand (ash bits -10) 255))
-			  result))))
+	    (setq result (cons (logand (ash bits -10) 255)
+			       result))))
 	  (skip-chars-forward non-data-chars end))
 	(if file-name
             (with-temp-file file-name
               (set-buffer-multibyte nil)
-              (insert (apply #'concat (nreverse result))))
+              (apply #'insert (nreverse result)))
 	  (or (markerp end) (setq end (set-marker (make-marker) end)))
 	  (goto-char start)
-	  (if enable-multibyte-characters
-	      (dolist (x (nreverse result))
-                (insert (decode-coding-string x 'binary)))
-	    (insert (apply #'concat (nreverse result))))
+          (apply #'insert
+                 (nreverse
+                  (if enable-multibyte-characters
+                      (mapcar (lambda (ch)
+                                (or (decode-char 'eight-bit ch) ch))
+                              result)
+                    result)))
 	  (delete-region (point) end))))))
 
 ;;;###autoload



-- 
Kazuhiro Ito

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Eli Zaretskii]

> Date: Thu, 05 Nov 2020 19:48:08 +0900
> From: Kazuhiro Ito <kzhr@d1.dion.ne.jp>
> Cc: 44411@debbugs.gnu.org
> 
> Here is a revised one.

Thanks, this LGTM.  I will wait for a couple of days, and push then in
your name if no new comments are voiced.

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Eli Zaretskii]

> Date: Thu, 05 Nov 2020 19:48:08 +0900
> From: Kazuhiro Ito <kzhr@d1.dion.ne.jp>
> Cc: 44411@debbugs.gnu.org
> 
> > Can you augment your patch along these lines, please?
> 
> Here is a revised one.

Thanks, pushed to the emacs-27 branch.

Since with this patch you have exhausted the amount of changes we can
accept from you without copyright assignment, would you like to start
the assignment process at this time?

bug#44411: 28.0.50; uudecode-decode-region-internal is broken

[Kazuhiro Ito]

> > > Can you augment your patch along these lines, please?
> > 
> > Here is a revised one.
> 
> Thanks, pushed to the emacs-27 branch.

Thank you.

> Since with this patch you have exhausted the amount of changes we can
> accept from you without copyright assignment, would you like to start
> the assignment process at this time?

I've stared the process and requested the assignment form.

-- 
Kazuhiro Ito