9669 Extra zeros sent by sendfile()

Reviews
Diff

- Download Diff

Information
Submitter:	Marcel Telka
Repository:	illumos-gate
Branch:	master
Bugs:	9669
Depends On:
Commit:	451cf09...
Reviewers
Groups:	general
People:

Description

This fixes a bug in the sendfile(3ext)/sendfilev(3ext) implementation where
some extra zeros could be written to output in a case we asked to copy more
data than there are in the input file.

Testing Done

Without the fix
---------------

# dtrace -n 'tcp_zcopy_check:return /pid==$target/ {trace(arg1)} snf_segmap:entry /pid==$target/ {stack();trace(arg3)}' -c ./test-64
dtrace: description 'tcp_zcopy_check:return ' matched 2 probes
sendfile call: off = 0, len = 8192
sendfile: ret = 8192, off = 8192, errno = 0
sendfile call: off = 20, len = 8192
sendfile: ret = 8192, off = 8212, errno = 0
dtrace: pid 675 has exited
CPU     ID                    FUNCTION:NAME
  0  57683           tcp_zcopy_check:return                 1
  0  59899                 snf_segmap:entry
              genunix`sendvec_chunk+0xb94
              genunix`sendfilev+0x36d
              unix`sys_syscall+0x177
             8192
  0  59899                 snf_segmap:entry
              genunix`sendvec_chunk+0xb94
              genunix`sendfilev+0x36d
              unix`sys_syscall+0x177
             8192

#


With the fix
------------

# dtrace -n 'tcp_zcopy_check:return /pid==$target/ {trace(arg1)} snf_segmap:entry /pid==$target/ {stack();trace(arg3)}' -c ./test-64
dtrace: description 'tcp_zcopy_check:return ' matched 2 probes
sendfile call: off = 0, len = 8192
sendfile: ret = 6, off = 6, errno = 0
sendfile call: off = 20, len = 8192
sendfile: ret = 0, off = 20, errno = 0
dtrace: pid 100698 has exited
CPU     ID                    FUNCTION:NAME
  0  49879           tcp_zcopy_check:return                 1
  0  52107                 snf_segmap:entry
              genunix`sendvec_chunk+0xbe3
              genunix`sendfilev+0x3ba
              unix`sys_syscall+0x27d
                6

#

Issues

0
0
1
All Issues: 1

Description	From	Last Updated
Is there a reason that we're performing this check outside of snf_segmap? Isn't this something that all callers of snf_segmap ...	Robert Mustacchi	July 24, 2018, 11:55 p.m.

could you also post the test-64 source in testing done or is it part of some test suite already?

Marcel Telka July 24, 2018, 10:24 a.m.

https://www.illumos.org/issues/9669#note-1

usr/src/uts/common/syscall/sendfile.c (Diff revision 1)

Show all issues

Is there a reason that we're performing this check outside of snf_segmap? Isn't this something that all callers of snf_segmap need to do and therefore we should not be sending data beyond that which we can read?

Given that we have checks in snf_segmap that seem to adjust the total size, is there a reason that we're not leveraging those and cleaning them up versus asking for a different length at this point?

Does sosendfile64 suffer from the same problem as it also calls snf_segmap?

Marcel Telka July 24, 2018, 11:55 p.m.

Callers of snf_segmap() (similarly as callers of snf_cache()) are expected to do the check before these functions are called.  Similarly as it is done in sosendfile64().  Yes, I noticed that we could leverage similar checks in both snf_segmap() and snf_cache() just by moving them at the begining of the loop (in both functions), but there would be a problem in sosendfile64() where we need to know the real size to transfer before the snf_segmap() is called, because based on the size we decide to call either snf_segmap() or snf_cache().  So I decided it is far easier just to follow the current habit and adjust the length before the snf_segmap() is called.
No, sosendfile64() does not suffer from the same problem as it does similar size check (and adjust) around line 2900 in socksyscalls.c.

Robert Mustacchi July 25, 2018, 12:24 a.m.

OK, that's reasoanble. Would you mind adding a comment to that effect to snf_segmap that we need to have adjusted the total size based on the vnode's size?

Marcel Telka July 25, 2018, 1:12 p.m.
```
Yes, this makes sense.  Done.
```

Change Summary:

Added comment to snf_segmap() (+ nits).

Commit:

-	83764bd7d5aa63a92db57815247e6e3f5b6ea2a4
+	451cf09be0c3d3804181f4e5a5258e230736d81a

Diff:

Revision 2 (+34 -31)

Show changes

	usr/src/uts/common/fs/sockfs/socksyscalls.c
	usr/src/uts/common/syscall/sendfile.c

Ship it!

Ship It!

Status: Closed (submitted)

Change Summary:

commit 8cd3131235b232e4d63be3cf95ce9be87907e74f
Author:     Marcel Telka <marcel@telka.sk>
AuthorDate: Wed Jul 25 12:06:18 2018 +0200
Commit:     Richard Lowe <richlowe@richlowe.net>
CommitDate: Mon Aug 6 19:38:36 2018 +0000

    9669 Extra zeros sent by sendfile()
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Approved by: Richard Lowe <richlowe@richlowe.net>

:100644 100644 21f3744895 4cbd079539 M	usr/src/uts/common/fs/sockfs/socksyscalls.c
:100644 100644 822f14416c 0cfafbf13f M	usr/src/uts/common/syscall/sendfile.c

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Change Summary:

Commit:

Diff:

Status: Closed (submitted)

Change Summary: