soreceive_stream: mbuf leak if called with mp0 and MSG_WAITALL
Andre Oppermann
andre at freebsd.org
Mon Mar 12 21:08:37 UTC 2012
On 08.03.2012 11:53, Mikolaj Golub wrote:
> Hi,
>
> On Tue, 06 Mar 2012 20:50:34 +0100 Andre Oppermann wrote:
>
> AO> On 05.09.2011 21:58, Mikolaj Golub wrote:
> >>
> >> On Sun, 04 Sep 2011 12:30:53 +0300 Mikolaj Golub wrote:
> >>
> >> MG> Apparently soreceive_stream() has an issue if it is called to receive data as a
> >> MG> mbuf chain (by supplying an non zero mbuf **mp0) and with MSG_WAITALL set.
> >>
> >> MG> I ran into this issue with smbfs, which uses soreceive() exactly in this way
> >> MG> (see netsmb/smb_trantcp.c:nbssn_recv()).
> >>
> >> Stressing smbfs a little I also observed the following soreceive_stream()
> >> related panic:
>
> AO> Hi Mikolaj,
>
> AO> thank you very much for testing, reporting and fixing bugs in soreceive_stream().
>
> AO> I've altered your proposed patches a bit and committed them into my workqueue
> AO> with the following revisions:
>
> AO> http://svn.freebsd.org/changeset/base/232617
> AO> http://svn.freebsd.org/changeset/base/232618
>
> AO> Would you mind testing them again before they go into HEAD?
>
> With this patch smb mount fails with the error:
>
> smb_iod_recvall: tran return NULL without error
>
> AO> Index: sys/kern/uipc_socket.c
> AO> ===================================================================
> AO> --- sys/kern/uipc_socket.c (revision 232616)
> AO> +++ sys/kern/uipc_socket.c (revision 232617)
> AO> @@ -2044,7 +2044,7 @@ deliver:
> AO> if (mp0 != NULL) {
> AO> /* Dequeue as many mbufs as possible. */
> AO> if (!(flags& MSG_PEEK)&& len>= sb->sb_mb->m_len) {
> AO> - for (*mp0 = m = sb->sb_mb;
> AO> + for (m = sb->sb_mb;
> AO> m != NULL&& m->m_len<= len;
> AO> m = m->m_next) {
> AO> len -= m->m_len;
> AO> @@ -2052,10 +2052,15 @@ deliver:
> AO> sbfree(sb, m);
> AO> n = m;
> AO> }
> AO> + n->m_next = NULL;
> AO> sb->sb_mb = m;
> AO> + sb->sb_lastrecord = sb->sb_mb;
> AO> if (sb->sb_mb == NULL)
> AO> SB_EMPTY_FIXUP(sb);
> AO> - n->m_next = NULL;
> AO> + if (*mp0 != NULL)
> AO> + m_cat(*mp0, m);
> AO> + else
> AO> + *mp0 = m;
> AO> }
>
> At that moment m points to the end of the chain. Shouldn't *mp0 be set to
> sb->sb_mb before the "for" loop?
Yes, doesn't compute this way. I've put in your fix in this revision:
http://svn.freebsd.org/changeset/base/232867
--
Andre
> AO> /* Copy the remainder. */
> AO> if (len> 0) {
> AO> @@ -2066,9 +2071,9 @@ deliver:
> AO> if (m == NULL)
> AO> len = 0; /* Don't flush data from sockbuf. */
> AO> else
> AO> - uio->uio_resid -= m->m_len;
> AO> + uio->uio_resid -= len;
> AO> if (*mp0 != NULL)
> AO> - n->m_next = m;
> AO> + m_cat(*mp0, m);
> AO> else
> AO> *mp0 = m;
> AO> if (*mp0 == NULL) {
> AO>
>
More information about the freebsd-net
mailing list