Sender: =?UTF-8?Q?Timo_Ter=C3=A4s?= <timo.teras@gmail.com>
Date: Mon, 6 Feb 2017 13:11:05 +0200
From: Timo Teras <timo.teras@iki.fi>
To: Huszty =?UTF-8?B?R2VyZ8WR?= <huszty.gergo@digitaltrip.hu>
Cc: alpine-aports@lists.alpinelinux.org
Subject: Re: [alpine-aports] [PATCH] community/minidlna: patch for potential
 segfaults
Message-ID: <20170206131105.6965c132@vostro.util.wtbts.net>
In-Reply-To: <bf917f6c59935b95472c9e2053bca52a@digitaltrip.hu>
References: <20170204201354.26019-1-huszty.gergo@digitaltrip.hu>
	<20170206114306.4920c40c@vostro.util.wtbts.net>
	<cb9fb84195a7e4a3ee85e726c3ff6f63@digitaltrip.hu>
	<bf917f6c59935b95472c9e2053bca52a@digitaltrip.hu>
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 06 Feb 2017 11:52:13 +0100
Huszty Gerg=C5=91 <huszty.gergo@digitaltrip.hu> wrote:

> Aport list added to CC.=20
>=20
> 2017-02-06 11:26 id=C5=91pontban Huszty Gerg=C5=91 ezt =C3=ADrta:
>=20
> > Please see my comments inline. So I didn't dig myself too much into
> > the details, but stack is exhausted always at this point on my
> > system and maybe other users suffer from the same issue.
> > I didn't want to touch the code too much and also didn't want to
> > increase the very typical 8192k stack ulimit size. I don't know how
> > typical is to set exclusive stack size for an app. Do you think that
> > would be a better solution?

The 8M ulimit based stack size affects only the main thread. The
program creates additional thread(s) in minidlna.c:1052. On those
threads the musl default is 80k. You should use
pthread_attr_setstacksize to change this.

For example, see:
http://git.alpinelinux.org/cgit/aports/tree/main/mpd/stacksize.patch

> > 2017-02-06 10:43 id=C5=91pontban Timo Teras ezt =C3=ADrta:
> >=20
> > On Sat,  4 Feb 2017 20:13:54 +0000
> > Gergo Huszty <huszty.gergo@digitaltrip.hu> wrote:
> >=20
> > From: libesz <huszty.gergo@digitaltrip.hu>
> >=20
> > Nfo parsing related fixes added in a patch.
> > - uninitalized string (GetVideoMetadata() - nfo) -> memset to 0
> > - stack was kicked with 64k buffer unconditionally (parse_nfo() -
> > buf) -> now it is on heap and malloc'd size depends on filesize ---
> > community/minidlna/10-minidlna-nfo.patch | 42
> > ++++++++++++++++++++++++++++++++
> > community/minidlna/APKBUILD              | 14 +++++++---- 2 files
> > changed, 51 insertions(+), 5 deletions(-) create mode 100644
> > community/minidlna/10-minidlna-nfo.patch
> >=20
> > diff --git a/community/minidlna/10-minidlna-nfo.patch
> > b/community/minidlna/10-minidlna-nfo.patch new file mode 100644
> > index 0000000..8e3497c
> > --- /dev/null
> > +++ b/community/minidlna/10-minidlna-nfo.patch
> > @@ -0,0 +1,42 @@
> > +--- a/metadata.c
> > ++++ b/metadata.c
> > +@@ -160,7 +160,7 @@
> > + parse_nfo(const char *path, metadata_t *m)
> > + {
> > +     FILE *nfo;
> > +-    char buf[65536];
> > ++    char *buf;
> > +     struct NameValueParserData xml;
> > +     struct stat file;
> > +     size_t nread;
> > +@@ -172,11 +172,13 @@
> > +         DPRINTF(E_INFO, L_METADATA, "Not parsing very
> > large .nfo file %s\n", path);
> > +         return;
> > +     }
> > ++    buf =3D malloc(file.st_size+1);=20
> > The return value should be checked for out-of-memory. =20
>  Yep, going to add it.=20
>=20
> > Additionally the code above checks for maximum of 65536, so using
> > malloc does not help much here. Large files seem to be ignored
> > intentionally.
> >=20
> > Though, if this fixes issues it's probably due to reducing stack
> > usage. It might be better to increase default stack size, as large
> > stack buffers might be used elsewhere. =20
>  Well that's interesting because the program fails every time exactly
> here.=20

Probably the inotify thread crashing due to out of stack.

> >> ++    memset(buf, '\0', file.st_size+1);
> >> +     DPRINTF(E_DEBUG, L_METADATA, "Parsing .nfo file: %s\n",
> >> path);
> >> +     nfo =3D fopen(path, "r");
> >> +     if( !nfo )
> >> +         return;
> >> +-    nread =3D fread(&buf, 1, sizeof(buf), nfo);
> >> ++    nread =3D fread(buf, 1, file.st_size, nfo);
> >> +
> >> +     ParseNameValue(buf, nread, &xml, 0);
> >> +
> >> +@@ -676,6 +679,7 @@
> >> +
> >> +     memset(&m, '\0', sizeof(m));
> >> +     memset(&video, '\0', sizeof(video));
> >> ++    memset(nfo, '\0', sizeof(nfo));
> >> +
> >> +     //DEBUG DPRINTF(E_DEBUG, L_METADATA, "Parsing video
> >> %s...\n", name);
> >> +     if ( stat(path, &file) !=3D 0 ) =20
> >=20
> > This probable does not fix anything. nfo is strcpy:ied to. =20
>  Yes it is, but the string was not null terminated so it was passed
> forward as a filepath with junk at the end. At least I saw it in the
> debugger.=20

That is normal. Debugger shows the whole char array. When used the
first zero byte terminates the string, and it'd handled right.


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---