Re: [xz-devel] [PATCH] LZMA_FINISH will now trigger LZMA_BUF_ERROR on truncated xz files right away

2022-08-10 Thread Lasse Collin 
On 2022-04-21 Jia Tan wrote:
> The current behavior of LZMA_FINISH in the decoder is a little
> confusing because it requires calling lzma_code a few times without
> providing more input to trigger a LZMA_BUF_ERROR.

The current behavior basically ignores the use LZMA_FINISH when
determining if LZMA_BUF_ERROR should be returned. I understand that it
can be confusing since after LZMA_FINISH there is nothing a new call to
lzma_code() can do to avoid the problem. However, I don't think it's a
problem in practice:

  - Application that calls lzma_code() in a loop will just call
lzma_code() again and eventually get LZMA_BUF_ERROR.

  - Application that does single-shot decoding without a loop tends to
check for LZMA_STREAM_END as a success condition and treats other
codes, including LZMA_OK, as a problem. In the worst case a less
robust application could break if this LZMA_OK becomes
LZMA_BUF_ERROR as the existing API doc says that LZMA_BUF_ERROR
won't be returned immediately. The docs don't give any
indication that LZMA_FINISH could affect this behavior.

  - An extra call or two to lzma_code() in an error condition doesn't
matter in terms of performance.

> This patch replaces return LZMA_OK lines with:
> 
> return action == LZMA_FINISH && *out_pos != out_size ? LZMA_BUF_ERROR
> : LZMA_OK;

I don't like replacing a short statement with a copy-pasted long
statement since it is needed in so many places. A benefit of the
current approach is that the handling of LZMA_BUF_ERROR is in
lzma_code() and (most of the time) the rest of code can ignore the
problem completely.

Also, the condition *out_pos != out_size is confusing in a few places.
For example, in SEQ_STREAM_HEADER:

--- a/src/liblzma/common/stream_decoder.c
+++ b/src/liblzma/common/stream_decoder.c
@@ -118,7 +118,8 @@ stream_decode(void *coder_ptr, const lzma_allocator 
*allocator,
 
// Return if we didn't get the whole Stream Header yet.
if (coder->pos < LZMA_STREAM_HEADER_SIZE)
-   return LZMA_OK;
+   return action == LZMA_FINISH && *out_pos != out_size
+   ? LZMA_BUF_ERROR : LZMA_OK;
 
coder->pos = 0;

In SEQ_STREAM_HEADER no output can be produced, only input will be
read. Still the condition checks for full output buffer which is not
only confusing but wrong: if there was an empty Stream ahead, having no
output space would be fine! In such a situation this can return LZMA_OK
even when the intention was to return LZMA_BUF_ERROR due to truncated
input. To make this work, only places that can produce output should
check if the output buffer is full.

However, I don't think the current behavior is worth changing. As you
pointed out, it is a bit weird (and I had never noticed it myself
before you mentioned it). It's not actually broken though and some
applications doing single-shot decoding might even rely on the current
behavior. Trying to change this could cause problems in rare cases and,
if not done carefully enough, introduce new bugs. So I thank you for
the patch but it won't be included.

-- 
Lasse Collin

[xz-devel] [PATCH] LZMA_FINISH will now trigger LZMA_BUF_ERROR on truncated xz files right away

2022-04-21 Thread Jia Tan 
The current behavior of LZMA_FINISH in the decoder is a little
confusing because it requires calling lzma_code a few times without
providing more input to trigger a LZMA_BUF_ERROR. This patch replaces
return LZMA_OK lines with:

return action == LZMA_FINISH && *out_pos != out_size ? LZMA_BUF_ERROR : LZMA_OK;

This checks if the action is LZMA_FINISH and the output is not full,
then return the LZMA_BUF_ERROR. If the output is full, LZMA_OK is
needed because the caller needs to provide more room for the coder to
write out.
I believe this solves the issue in all cases and does not return
LZMA_BUF_ERROR when it shouldn't.

Jia Tan
From c7ebdb16a4f14fed641d82e97331eb39c4f57d1d Mon Sep 17 00:00:00 2001
From: jiat75 
Date: Thu, 21 Apr 2022 21:09:57 +0800
Subject: [PATCH] LZMA_FINISH will now trigger LZMA_BUF_ERROR on truncated xz files right away

 In the single threaded and multithreaded decoder, LZMA_FINISH
 will cause BUF_ERROR right away instead of requiring multiple calls to
 lzma_code without providing more input.

---
 src/liblzma/common/common.c|  1 -
 src/liblzma/common/stream_decoder.c| 19 ++-
 src/liblzma/common/stream_decoder_mt.c | 19 ++-
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/liblzma/common/common.c b/src/liblzma/common/common.c
index 346fc7af..3ddaf199 100644
--- a/src/liblzma/common/common.c
+++ b/src/liblzma/common/common.c
@@ -352,7 +352,6 @@ lzma_code(lzma_stream *strm, lzma_action action)
 
 	default:
 		// All the other errors are fatal; coding cannot be continued.
-		assert(ret != LZMA_BUF_ERROR);
 		strm->internal->sequence = ISEQ_ERROR;
 		break;
 	}
diff --git a/src/liblzma/common/stream_decoder.c b/src/liblzma/common/stream_decoder.c
index fdd8ff2f..06d4136b 100644
--- a/src/liblzma/common/stream_decoder.c
+++ b/src/liblzma/common/stream_decoder.c
@@ -118,7 +118,8 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 
 		// Return if we didn't get the whole Stream Header yet.
 		if (coder->pos < LZMA_STREAM_HEADER_SIZE)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		coder->pos = 0;
 
@@ -161,7 +162,8 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 
 	case SEQ_BLOCK_HEADER: {
 		if (*in_pos >= in_size)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		if (coder->pos == 0) {
 			// Detect if it's Index.
@@ -184,7 +186,8 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 
 		// Return if we didn't get the whole Block Header yet.
 		if (coder->pos < coder->block_options.header_size)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		coder->pos = 0;
 
@@ -256,6 +259,10 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 in, in_pos, in_size, out, out_pos, out_size,
 action);
 
+		if (ret == LZMA_OK)
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
+
 		if (ret != LZMA_STREAM_END)
 			return ret;
 
@@ -275,7 +282,8 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 		// lzma_index_hash_decode() since it would return
 		// LZMA_BUF_ERROR, which we must not do here.
 		if (*in_pos >= in_size)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		// Decode the Index and compare it to the hash calculated
 		// from the sizes of the Blocks (if any).
@@ -296,7 +304,8 @@ stream_decode(void *coder_ptr, const lzma_allocator *allocator,
 
 		// Return if we didn't get the whole Stream Footer yet.
 		if (coder->pos < LZMA_STREAM_HEADER_SIZE)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		coder->pos = 0;
 
diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
index e8939254..1519e570 100644
--- a/src/liblzma/common/stream_decoder_mt.c
+++ b/src/liblzma/common/stream_decoder_mt.c
@@ -1053,7 +1053,8 @@ stream_decode_mt(void *coder_ptr, const lzma_allocator *allocator,
 
 		// Return if we didn't get the whole Stream Header yet.
 		if (coder->pos < LZMA_STREAM_HEADER_SIZE)
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 
 		coder->pos = 0;
 
@@ -1153,7 +1154,8 @@ stream_decode_mt(void *coder_ptr, const lzma_allocator *allocator,
 break;
 			}
 
-			return LZMA_OK;
+			return action == LZMA_FINISH && *out_pos != out_size
+	? LZMA_BUF_ERROR : LZMA_OK;
 		}
 
 		if (ret == LZMA_INDEX_DETECTED) {
@@ -1513,7 +1515,8 @@ stream_decode_mt(void *coder_ptr, const lzma_allocator *allocator,
 		// Return if the input didn't contain the whole Block.
 		if (coder->thr->in_filled < coder->thr->in_size) {
 			assert(*in_pos == in_size);
-			return LZMA_OK;
+			return action ==