Mail archive
alpine-aports

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

From: Huszty Gergő <huszty.gergo_at_digitaltrip.hu>
Date: Mon, 06 Feb 2017 11:52:13 +0100

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_at_digitaltrip.hu> wrote:
>
> From: libesz <huszty.gergo_at_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
> _at__at_ -0,0 +1,42 @@
> +--- a/metadata.c
> ++++ b/metadata.c
> +_at__at_ -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;
> +_at__at_ -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);
>> +
>> +_at__at_ -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.

>> _at__at_ -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.


---
Unsubscribe:  alpine-aports+unsubscribe_at_lists.alpinelinux.org
Help:         alpine-aports+help_at_lists.alpinelinux.org
---
Received on Mon Feb 06 2017 - 11:52:13 GMT