2 2

Re: [alpine-aports] [PATCH] community/minidlna: patch for potential segfaults

Huszty Gergő
Details
Message ID
<bf917f6c59935b95472c9e2053bca52a@digitaltrip.hu>
Sender timestamp
1486378333
DKIM signature
missing
Download raw message
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

Timo Teras
Details
Message ID
<20170206131105.6965c132@vostro.util.wtbts.net>
In-Reply-To
<bf917f6c59935b95472c9e2053bca52a@digitaltrip.hu> (view parent)
Sender timestamp
1486379465
DKIM signature
missing
Download raw message
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

Gergo Huszty
Details
Message ID
<20170206134339.17127-1-huszty.gergo@digitaltrip.hu>
In-Reply-To
<20170206131105.6965c132@vostro.util.wtbts.net> (view parent)
Sender timestamp
1486388619
DKIM signature
missing
Download raw message
Patch: +41 -5
As suggested, stack size is increased instead of moving nfo file
buffer to heap.
---
 community/minidlna/10-minidlna-nfo.patch | 32 ++++++++++++++++++++++++++++++++
 community/minidlna/APKBUILD              | 14 +++++++++-----
 2 files changed, 41 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..80fdab1
--- /dev/null
+++ b/community/minidlna/10-minidlna-nfo.patch
@@ -0,0 +1,32 @@
+--- a/metadata.c
+@@ -676,6 +676,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 )
+--- a/minidlna.c
+@@ -1049,8 +1049,17 @@
+ 		if (!sqlite3_threadsafe() || sqlite3_libversion_number() < 3005001)
+ 			DPRINTF(E_ERROR, L_GENERAL, "SQLite library is not threadsafe!  "
+ 			                            "Inotify will be disabled.\n");
+-		else if (pthread_create(&inotify_thread, NULL, start_inotify, NULL) != 0)
+-			DPRINTF(E_FATAL, L_GENERAL, "ERROR: pthread_create() failed for start_inotify. EXITING\n");
++		else
++		{
++			pthread_attr_t attr, *attrptr = NULL;
++			if ((pthread_attr_init(&attr) == 0) && (pthread_attr_setstacksize(&attr, 192 * 1024) == 0))
++				attrptr = &attr;
++			else
++				DPRINTF(E_ERROR, L_GENERAL, "Failed to set inotify thread stack size,"
++							    "continuing with the default.\n");
++			if (pthread_create(&inotify_thread, attrptr, start_inotify, NULL) != 0)
++				DPRINTF(E_FATAL, L_GENERAL, "ERROR: pthread_create() failed for start_inotify. EXITING\n");
++		}
+ 	}
+ #endif
+ 	smonitor = OpenAndConfMonitorSocket();
diff --git a/community/minidlna/APKBUILD b/community/minidlna/APKBUILD
index cab6b58..d296776 100644
--- a/community/minidlna/APKBUILD
+++ b/community/minidlna/APKBUILD
@@ -2,12 +2,13 @@
 # Maintainer: Francesco Colista <francesco.colista@gmail.com>
 pkgname=minidlna
 pkgver=1.1.5
-pkgrel=3
+pkgrel=4
 pkgdesc="A small dlna server"
 url="http://sourceforge.net/projects/minidlna/"
 arch="all"
 license="GPL"
 depends=
+options=
 makedepends="
 	bsd-compat-headers
 	libvorbis-dev
@@ -26,7 +27,7 @@ pkggroups="$pkgname"
 source="http://downloads.sourceforge.net/project/minidlna/minidlna/$pkgver/minidlna-$pkgver.tar.gz
 	$pkgname.initd
 	$pkgname.confd
-	"
+	10-minidlna-nfo.patch"
 
 builddir="$srcdir/$pkgname-$pkgver"
 
@@ -63,10 +64,13 @@ package() {
 
 md5sums="1970e553a1eb8a3e7e302e2ce292cbc4  minidlna-1.1.5.tar.gz
 6dd1ec5560ac30d7a04244101e912d45  minidlna.initd
-59d14c1bf3cd637138bfa58db7255d78  minidlna.confd"
+59d14c1bf3cd637138bfa58db7255d78  minidlna.confd
+d4ae54c1249b2263646a68afc32aae62  10-minidlna-nfo.patch"
 sha256sums="8477ad0416bb2af5cd8da6dde6c07ffe1a413492b7fe40a362bc8587be15ab9b  minidlna-1.1.5.tar.gz
 251e790bb8adb91b4d00dd47543b9b02409879ade0853ebc3bd0fc0184bd485e  minidlna.initd
-67603d65c6bd3918255f050cb5cfd6fc1373b024bca1ce728f03491a90d79e19  minidlna.confd"
+67603d65c6bd3918255f050cb5cfd6fc1373b024bca1ce728f03491a90d79e19  minidlna.confd
+d060e1181277f3b31a8cc64050cfc9a2ce6d1891b6bf81b4e8463d5e0e450891  10-minidlna-nfo.patch"
 sha512sums="2a8eaa42fcda6f98648f1726af5cdba6d2358c386440dd0de933364cfbd1ced2fee5f883033e1a5a692b760749beb2c12798020a3591ddcea22663102d4f3dfa  minidlna-1.1.5.tar.gz
 e16961bb68c004297f1e26422b1d15bd8583ba2e0e36c88902a45573b685993fff88d2d0dae8c624eaeddb0deca614dbc13b8345f34b4c348961c00b05c0df30  minidlna.initd
-e209848af0d79069ac989ad61d3be610b4c0c2783a207a50463a25ec3811b04d1da3a2acde54749878bec44e1567874ede827b978d5472c00f6a855663e5cbf8  minidlna.confd"
+e209848af0d79069ac989ad61d3be610b4c0c2783a207a50463a25ec3811b04d1da3a2acde54749878bec44e1567874ede827b978d5472c00f6a855663e5cbf8  minidlna.confd
+59a97ef0a36d3ae44dd2e182a0b106f84ce5c17e7dc14ee0459b17430b57ddc59a74e8e67fc0a90326fa451a505b97a0b719b438475efac144028dd012b44af1  10-minidlna-nfo.patch"
-- 
2.8.3



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---