[PATCH 1/1] cli/show: avoid empty write to stdout in format_part_raw

[PATCH 1/1] cli/show: avoid empty write to stdout in format_part_raw

[Rob Browning]

Previously (at least) if the input was exactly 4096 bytes long,
notmuch would attempt to fwrite nothing to stdout, but still expected
fwrite to return 1, causing a failure that looked like this:

  $ notmuch show --format=raw id:87o96f1cya.fsf@codeaurora.org
    ...entire message shown as expected..
  Error: Write failed
  $ echo $?
  1

To fix the problem don't call fwrite at all when there's nothing to
write.
---

 Proposed for 0.28.4 -- thanks.

 notmuch-show.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index c3a3783a..98b85352 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -851,7 +851,7 @@ format_part_raw (unused (const void *ctx), unused (sprinter_t *sp),
 		return NOTMUCH_STATUS_FILE_ERROR;
 	    }
 
-	    if (fwrite (buf, size, 1, stdout) != 1) {
+	    if (size > 0 && fwrite (buf, size, 1, stdout) != 1) {
 		fprintf (stderr, "Error: Write failed\n");
 		fclose (file);
 		return NOTMUCH_STATUS_FILE_ERROR;
-- 
2.20.1

v2 format_raw fix

[David Bremner]

This series just adds tests for Rob's fix. I'm resending Rob's patch as well because I
added a single deletion (of the known_broken test).

[PATCH 1/2] test: notmuch show --format=raw for 4096 byte messages.

[David Bremner]

Rob Browning isolated a bug where files of exactly 4096 bytes generate
errors because of a zero byte read.
---
 test/T210-raw.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/test/T210-raw.sh b/test/T210-raw.sh
index 99fdef72..e1dd6064 100755
--- a/test/T210-raw.sh
+++ b/test/T210-raw.sh
@@ -30,4 +30,33 @@ Date: GENERATED_DATE
 
 This is just a test message (#2)"
 
+test_begin_subtest "generating 4096 byte message"
+line=$(seq 1 72 | sed 's/.*/./' | tr -d '\n')
+for i in $(seq 1 53); do
+    printf "%s\n" "$line" >> BODY
+done
+# The last newline is added by add_message
+printf "01234567890123456789012345" >> BODY
+add_message '[subject]="4096"' '[body]="$(cat BODY)"'
+output="$((`wc -c < $gen_msg_filename`))"
+test_expect_equal 4096 "$output"
+
+test_begin_subtest "Show a raw message exactly 4096 bytes: no error"
+test_subtest_known_broken
+test_expect_success "notmuch show --format=raw subject:4096 1>/dev/null"
+
+test_begin_subtest "Show a raw message exactly 4096 bytes: correct output"
+notmuch show --format=raw subject:4096 | notmuch_date_sanitize > OUTPUT
+cat <<EOF > EXPECTED
+From: Notmuch Test Suite <test_suite@notmuchmail.org>
+To: Notmuch Test Suite <test_suite@notmuchmail.org>
+Message-Id: <msg-003@notmuch-test-suite>
+Subject: 4096
+Date: GENERATED_DATE
+
+EOF
+cat BODY >> EXPECTED
+printf "\n" >> EXPECTED
+test_expect_equal_file EXPECTED OUTPUT
+
 test_done
-- 
2.20.1

[PATCH 2/2] cli/show: avoid empty write to stdout in format_part_raw

[David Bremner]

From: Rob Browning <rlb@defaultvalue.org>

Previously (at least) if the input was exactly 4096 bytes long,
notmuch would attempt to fwrite nothing to stdout, but still expected
fwrite to return 1, causing a failure that looked like this:

  $ notmuch show --format=raw id:87o96f1cya.fsf@codeaurora.org
    ...entire message shown as expected..
  Error: Write failed
  $ echo $?
  1

To fix the problem don't call fwrite at all when there's nothing to
write.
---
 notmuch-show.c   | 2 +-
 test/T210-raw.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 88699e90..30363555 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -855,7 +855,7 @@ format_part_raw (unused (const void *ctx), unused (sprinter_t *sp),
 		return NOTMUCH_STATUS_FILE_ERROR;
 	    }
 
-	    if (fwrite (buf, size, 1, stdout) != 1) {
+	    if (size > 0 && fwrite (buf, size, 1, stdout) != 1) {
 		fprintf (stderr, "Error: Write failed\n");
 		fclose (file);
 		return NOTMUCH_STATUS_FILE_ERROR;
diff --git a/test/T210-raw.sh b/test/T210-raw.sh
index e1dd6064..ee78e664 100755
--- a/test/T210-raw.sh
+++ b/test/T210-raw.sh
@@ -42,7 +42,6 @@ output="$((`wc -c < $gen_msg_filename`))"
 test_expect_equal 4096 "$output"
 
 test_begin_subtest "Show a raw message exactly 4096 bytes: no error"
-test_subtest_known_broken
 test_expect_success "notmuch show --format=raw subject:4096 1>/dev/null"
 
 test_begin_subtest "Show a raw message exactly 4096 bytes: correct output"
-- 
2.20.1

Re: [PATCH 1/2] test: notmuch show --format=raw for 4096 byte messages.

[Tomi Ollila]

On Sun, Apr 28 2019, David Bremner wrote:

> Rob Browning isolated a bug where files of exactly 4096 bytes generate
> errors because of a zero byte read.

This happens to be effective test in case where FILE buffering uses 4096
byte buffers. If it used any other size then this is null test. So this
test expects implementation detail of stdio (so IMO testing this is waste
of time).

What could be useful, is that we just happen to have 4096 byte file in
our test corpus, and some tests which test other functionality could 
accidentally fail due to this bug. We could also have 1024, 2048,
8192, 16384 bytes (we still cannot say how large buffers any stdio
buffering uses but... :D)

And if such messages were generated, instead of add_message those could
be written "verbatim" from script... so that the file size is not
dependent how add_message creates it...

Tomi

PS: uh, our test code gives always headache to me... just was^H^H^Hspent
hour to look and think the code below. I wonder whether i could spend
my time better than thinking these....


>  1 file changed, 29 insertions(+)
>
> diff --git a/test/T210-raw.sh b/test/T210-raw.sh
> index 99fdef72..e1dd6064 100755
> --- a/test/T210-raw.sh
> +++ b/test/T210-raw.sh
> @@ -30,4 +30,33 @@ Date: GENERATED_DATE
>  
>  This is just a test message (#2)"
>  
> +test_begin_subtest "generating 4096 byte message"
> +line=$(seq 1 72 | sed 's/.*/./' | tr -d '\n')
> +for i in $(seq 1 53); do
> +    printf "%s\n" "$line" >> BODY
> +done
> +# The last newline is added by add_message
> +printf "01234567890123456789012345" >> BODY
> +add_message '[subject]="4096"' '[body]="$(cat BODY)"'
> +output="$((`wc -c < $gen_msg_filename`))"
> +test_expect_equal 4096 "$output"
> +
> +test_begin_subtest "Show a raw message exactly 4096 bytes: no error"
> +test_subtest_known_broken
> +test_expect_success "notmuch show --format=raw subject:4096 1>/dev/null"
> +
> +test_begin_subtest "Show a raw message exactly 4096 bytes: correct output"
> +notmuch show --format=raw subject:4096 | notmuch_date_sanitize > OUTPUT
> +cat <<EOF > EXPECTED
> +From: Notmuch Test Suite <test_suite@notmuchmail.org>
> +To: Notmuch Test Suite <test_suite@notmuchmail.org>
> +Message-Id: <msg-003@notmuch-test-suite>
> +Subject: 4096
> +Date: GENERATED_DATE
> +
> +EOF
> +cat BODY >> EXPECTED
> +printf "\n" >> EXPECTED
> +test_expect_equal_file EXPECTED OUTPUT
> +
>  test_done
> -- 
> 2.20.1

Re: [PATCH 1/2] test: notmuch show --format=raw for 4096 byte messages.

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Sun, Apr 28 2019, David Bremner wrote:
>
>> Rob Browning isolated a bug where files of exactly 4096 bytes generate
>> errors because of a zero byte read.
>
> This happens to be effective test in case where FILE buffering uses 4096
> byte buffers. If it used any other size then this is null test. So this
> test expects implementation detail of stdio (so IMO testing this is waste
> of time).

It's nice to show the next patch actually fixes something.

> What could be useful, is that we just happen to have 4096 byte file in
> our test corpus, and some tests which test other functionality could 
> accidentally fail due to this bug. We could also have 1024, 2048,
> 8192, 16384 bytes (we still cannot say how large buffers any stdio
> buffering uses but... :D)

One problem is that adding new messages to our standard corpus is a
messy operation, and I'm trying to keep the source diff small here.

>
> And if such messages were generated, instead of add_message those could
> be written "verbatim" from script... so that the file size is not
> dependent how add_message creates it...
>

Sure, that might make sense, although the script starts to bloat a bit
for the 16k messages. OTOH, I'm fine with replacing the add_message with
a literal 4k message for now.  

I guess we could actually test messages of size BUFSIZ. That isn't
guaranteed to be the actual buffer size, but it seems a better bet than
4096.

v3 raw output of messages with size multiples of stdio buffer size

[David Bremner]

Based on feedback from Tomi, this tests a more comprehensive set of
messages size (the generation is also less mysterious, if more
verbose). I also updated the commit message of Rob's patch slightly to
point to the possibility of other buffer sizes causing problems.

[PATCH 1/2] test/raw: add some messages likely to be multiples of buffer size

[David Bremner]

This highlights a bug where zero bytes are read at the end of the
file, and we attempt to write those to stdout, causing spurious write
failures.
---
 test/T210-raw.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/test/T210-raw.sh b/test/T210-raw.sh
index 99fdef72..0d57deb5 100755
--- a/test/T210-raw.sh
+++ b/test/T210-raw.sh
@@ -30,4 +30,39 @@ Date: GENERATED_DATE
 
 This is just a test message (#2)"
 
+test_python <<EOF
+from email.message import EmailMessage
+for pow in range(12,21):
+    size = 2**pow
+    msg=EmailMessage()
+    msg['Subject'] = 'message with {:07d} bytes'.format(size)
+    msg['From'] = 'Notmuch Test Suite <test_suite@notmuchmail.org>'
+    msg['To'] = msg['From']
+    msg['Message-Id'] = 'size-{0:07d}@notmuch-test-suite'.format(size)
+    content=""
+    msg.set_content("")
+    padding = size - len(bytes(msg))
+    lines = []
+    while padding > 0:
+        line = '.' * min(padding, 72)
+        lines.append(line)
+        padding = padding - len(line) - 1
+    content ='\n'.join(lines)
+    msg.set_content(content)
+    with open('mail/size-{0:07d}'.format(size),'wb') as f:
+        f.write(bytes(msg))
+EOF
+
+notmuch new --quiet
+
+for pow in $(seq 12 20); do
+    size=$(printf "%07d" $((2**$pow)))
+    test_begin_subtest "content, message of size $size"
+    notmuch show --format=raw subject:$size > OUTPUT
+    test_expect_equal_file mail/size-$size OUTPUT
+    test_begin_subtest "return value, message of size $size"
+    test_subtest_known_broken
+    test_expect_success  "notmuch show --format=raw subject:$size > /dev/null"
+done
+
 test_done
-- 
2.20.1

[PATCH 2/2] cli/show: avoid empty write to stdout in format_part_raw

[David Bremner]

From: Rob Browning <rlb@defaultvalue.org>

Previously if the input was exactly a multiple of the internal buffer
size, notmuch would attempt to fwrite nothing to stdout, but still
expected fwrite to return 1, causing a failure that looked like this:

  $ notmuch show --format=raw id:87o96f1cya.fsf@codeaurora.org
    ...entire message shown as expected..
  Error: Write failed
  $ echo $?
  1

To fix the problem don't call fwrite at all when there's nothing to
write.
---
 notmuch-show.c   | 2 +-
 test/T210-raw.sh | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index c3a3783a..98b85352 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -851,7 +851,7 @@ format_part_raw (unused (const void *ctx), unused (sprinter_t *sp),
 		return NOTMUCH_STATUS_FILE_ERROR;
 	    }
 
-	    if (fwrite (buf, size, 1, stdout) != 1) {
+	    if (size > 0 && fwrite (buf, size, 1, stdout) != 1) {
 		fprintf (stderr, "Error: Write failed\n");
 		fclose (file);
 		return NOTMUCH_STATUS_FILE_ERROR;
diff --git a/test/T210-raw.sh b/test/T210-raw.sh
index 0d57deb5..93944491 100755
--- a/test/T210-raw.sh
+++ b/test/T210-raw.sh
@@ -61,7 +61,6 @@ for pow in $(seq 12 20); do
     notmuch show --format=raw subject:$size > OUTPUT
     test_expect_equal_file mail/size-$size OUTPUT
     test_begin_subtest "return value, message of size $size"
-    test_subtest_known_broken
     test_expect_success  "notmuch show --format=raw subject:$size > /dev/null"
 done
 
-- 
2.20.1

Re: [PATCH 1/2] test/raw: add some messages likely to be multiples of buffer size

[Tomi Ollila]

On Sat, May 04 2019, David Bremner wrote:

> This highlights a bug where zero bytes are read at the end of the
> file, and we attempt to write those to stdout, causing spurious write
> failures.
> ---
>  test/T210-raw.sh | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/test/T210-raw.sh b/test/T210-raw.sh
> index 99fdef72..0d57deb5 100755
> --- a/test/T210-raw.sh
> +++ b/test/T210-raw.sh
> @@ -30,4 +30,39 @@ Date: GENERATED_DATE
>  
>  This is just a test message (#2)"
>  
> +test_python <<EOF
> +from email.message import EmailMessage
> +for pow in range(12,21):

.......................^ space. i.e. (12, 21)

from 4KiB to 1MiB. uh, perhaps I should have kept my fingers crossed.
(that said, my range would have been 10, 17 (1024 - 65536))

> +    size = 2**pow

spaces around '**'

> +    msg=EmailMessage()

spaces around '='

> +    msg['Subject'] = 'message with {:07d} bytes'.format(size)
> +    msg['From'] = 'Notmuch Test Suite <test_suite@notmuchmail.org>'
> +    msg['To'] = msg['From']
> +    msg['Message-Id'] = 'size-{0:07d}@notmuch-test-suite'.format(size)

for consistencty, size-{:07d} (python 2.7+ supported format) ...

> +    content=""

spaces

> +    msg.set_content("")
> +    padding = size - len(bytes(msg))
> +    lines = []
> +    while padding > 0:
> +        line = '.' * min(padding, 72)
> +        lines.append(line)
> +        padding = padding - len(line) - 1
> +    content ='\n'.join(lines)
> +    msg.set_content(content)
> +    with open('mail/size-{0:07d}'.format(size),'wb') as f:

size-{:07d} , space before 'wb'

> +        f.write(bytes(msg))
> +EOF
> +
> +notmuch new --quiet
> +
> +for pow in $(seq 12 20); do

{12..20} would do the above seq, w/o 2 forks and one execve :D

> +    size=$(printf "%07d" $((2**$pow)))

printf -v size "%07d" $((2 ** pow))

> +    test_begin_subtest "content, message of size $size"
> +    notmuch show --format=raw subject:$size > OUTPUT
> +    test_expect_equal_file mail/size-$size OUTPUT
> +    test_begin_subtest "return value, message of size $size"
> +    test_subtest_known_broken
> +    test_expect_success  "notmuch show --format=raw subject:$size > /dev/null"
> +done
> +
>  test_done
> -- 
> 2.20.1

Re: [PATCH 1/2] test/raw: add some messages likely to be multiples of buffer size

[Tomi Ollila]

On Sun, May 05 2019, Tomi Ollila wrote:

> On Sat, May 04 2019, David Bremner wrote:
>
>>  
>> +test_python <<EOF
>> +from email.message import EmailMessage
>> +for pow in range(12,21):
>
> .......................^ space. i.e. (12, 21)
>
> from 4KiB to 1MiB. uh, perhaps I should have kept my fingers crossed.
> (that said, my range would have been 10, 17 (1024 - 65536))

except that...

if fread(3) (and friends) used 1024 byte buffer, for 4096 byte message
it would read full buffer 4 times (and not yet noticing EOF), and then
next fread() would return 0 (and now flag EOF)...

... and then the same would apply for 4096 byte message and we could
just test 65536 byte message.

(so, one alternative could be testing e.g. 4096 and 65536 byte messages)

linux namual page does not exactly say, whether reading full request
is "guaranteed", but 2 other documents found in the wild said:

" The total number of elements successfully read is returned.
    
  If this number differs from the count parameter, either a reading error
  occurred or the end-of-file was reached while reading. In both cases, the
  proper indicator is set, which can be checked with ferror and feof,
  respectively."

(so the fix could be different in other email, but at least it is short :D)

... anyway, fread could internally read(2) less than BUFSIZ, copy that to
output buffer and then read(2) more... but at least most of the times
this test in question can protect against regressions in this particular
case)


Tomi

Re: [PATCH 1/2] test/raw: add some messages likely to be multiples of buffer size

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Sun, May 05 2019, Tomi Ollila wrote:
>
>> On Sat, May 04 2019, David Bremner wrote:
>>
>>>  
>>> +test_python <<EOF
>>> +from email.message import EmailMessage
>>> +for pow in range(12,21):
>>
>> .......................^ space. i.e. (12, 21)
>>
>> from 4KiB to 1MiB. uh, perhaps I should have kept my fingers crossed.
>> (that said, my range would have been 10, 17 (1024 - 65536))
>
> except that...
>
> if fread(3) (and friends) used 1024 byte buffer, for 4096 byte message
> it would read full buffer 4 times (and not yet noticing EOF), and then
> next fread() would return 0 (and now flag EOF)...

I originally started from a 1024 byte message, but it was a bit messy to
mark some sizes as broken and others not. Perhaps that's not a good
reason in retrospect, since the the fact that >= 4096 is broken is
specific to some libc (and kernel?). This means that if
someome has a libc where the buffer size is bigger then 4096, this test
might return non-zero.  My take away from that is that I should probably
squash these two commits into one.

d

Re: [PATCH 1/2] test/raw: add some messages likely to be multiples of buffer size

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

On Sun 2019-05-05 07:49:16 -0300, David Bremner wrote:
> My take away from that is that I should probably squash these two
> commits into one.

I've read this series and it seems reasonable to me.  i agree that we
should be testing messages smaller than 4096 octets, either by squashing
the two commits into one, or by only conditionally marking the tests as
known-broken.  it's not too bad to mark it known-broken conditionally if
you want to do the two-stage commit:

 [ size -lt 4096 ] || test_subtest_known_broken

but it's a little weird, because it's only *actually* "known broken"
depending on your platform's buffer size -- i could test this series in
2030 on debian GNU/Hurd 15 and find that because our buffer size is now
65536 none of the tests are actually broken yet :P

I also learned some extra bash from Tomi's nit-picking review, for which
i'm always grateful.  thanks, Tomi! :)  And thanks Rob for finding this!

Please merge this series.

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]