Aport list added to CC.
2017-02-06 11:26 időpontban Huszty Gergő ezt írta:
> Hi,> 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?> > Thanks,> Gergo> > 2017-02-06 10:43 időpontban Timo Teras ezt írta:> > On Sat, 4 Feb 2017 20:13:54 +0000> Gergo Huszty <huszty.gergo@digitaltrip.hu> wrote:> > From: libesz <huszty.gergo@digitaltrip.hu>> > 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> > 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 = malloc(file.st_size+1); > The return value should be checked for out-of-memory.
Yep, going to add it.
> Additionally the code above checks for maximum of 65536, so using> malloc does not help much here. Large files seem to be ignored> intentionally.> > 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.
Well that's interesting because the program fails every time exactly
here.
>> ++ memset(buf, '\0', file.st_size+1);>> + DPRINTF(E_DEBUG, L_METADATA, "Parsing .nfo file: %s\n",>> path);>> + nfo = fopen(path, "r");>> + if( !nfo )>> + return;>> +- nread = fread(&buf, 1, sizeof(buf), nfo);>> ++ nread = 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) != 0 )> > This probable does not fix anything. nfo is strcpy:ied to.
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.
>> @@ -63,10 +64,13 @@ package() {>> >> md5sums="1970e553a1eb8a3e7e302e2ce292cbc4 minidlna-1.1.5.tar.gz>> 6dd1ec5560ac30d7a04244101e912d45 minidlna.initd>> -59d14c1bf3cd637138bfa58db7255d78 minidlna.confd">> +59d14c1bf3cd637138bfa58db7255d78 minidlna.confd>> +ddb2a414261109509a81e2ede03e12ba 10-minidlna-nfo.patch">> sha256sums="8477ad0416bb2af5cd8da6dde6c07ffe1a413492b7fe40a362bc8587be15ab9b> > Checksum did not match. There was changes to it after last 'abuild> checksum' ?
Only tweaked the file paths a bit in the diff.
Re: [alpine-aports] [PATCH] community/minidlna: patch for potential segfaults
On Mon, 06 Feb 2017 11:52:13 +0100
Huszty Gergő <huszty.gergo@digitaltrip.hu> wrote:
> Aport list added to CC. > > 2017-02-06 11:26 időpontban Huszty Gergő ezt írta:> > > 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őpontban Timo Teras ezt írta:> > > > On Sat, 4 Feb 2017 20:13:54 +0000> > Gergo Huszty <huszty.gergo@digitaltrip.hu> wrote:> > > > From: libesz <huszty.gergo@digitaltrip.hu>> > > > 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> > > > 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 = malloc(file.st_size+1); > > The return value should be checked for out-of-memory. > Yep, going to add it. > > > Additionally the code above checks for maximum of 65536, so using> > malloc does not help much here. Large files seem to be ignored> > intentionally.> > > > 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. > Well that's interesting because the program fails every time exactly> here.
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 = fopen(path, "r");> >> + if( !nfo )> >> + return;> >> +- nread = fread(&buf, 1, sizeof(buf), nfo);> >> ++ nread = 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) != 0 ) > > > > This probable does not fix anything. nfo is strcpy:ied to. > 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.
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
---
[alpine-aports] [PATCH] community/minidlna: out of stack fix reworked