GNOME Bugzilla – Bug 740052
Crash in the dLeyna plugin
Last modified: 2014-11-27 16:04:55 UTC
The dLeyna plugin can crash Grilo. Look at this code from grl-dleyna-utils.c: /* ... */ status = g_io_channel_read_line (file, &line, NULL, NULL, NULL); while (status == G_IO_STATUS_NORMAL) { char *line; int j, k, l; /* 4*8 for IP, 4 for port, 1 for :, 1 for NUL */ char buffer[4*8 + 4 + 1 + 1]; guint32 ip[4]; guint16 port; guint32 all_ipv6[4] = { 0, 0, 0, 0 }; j = 0; /* skip leading space */ while (line[j] == ' ') j++; /* ... */ The "skip leading space" part is supposed to parse the line previously read with g_io_channel_read_line(), however we are redefining the 'line' variable inside the while loop. I wonder how come this does not crash more often. On top of that, the " while(line[j] == ' ') j++ " bit is not checking for the string terminator, so that's also a place for bad surprises.
Created attachment 290613 [details] [review] dleyna: Remove redefinition
> On top of that, the " while(line[j] == ' ') j++ " bit is not > checking for the string terminator The problem is actually in the " while (line[j] != ' ') j++ " that comes next, but my point is that I don't think it's a good idea to make this kind of assumptions when reading from an external file, even when its format is not supposed to change.
I'd take a patch with a test, which we could feed broken files to. At the very least a file that triggers crashes with the above problems.
Review of attachment 290613 [details] [review]: The commit message in this patch is pretty useless. How about: " dleyna: Fix possible crasher determining local IPv6 address Avoid a crash when determining the local IPv6 address due to a shadowed variable "
Created attachment 290835 [details] [review] dleyna: Fix crash determining local IPv6 address Avoid a crash when determining the local IPv6 address due to a shadowed variable.
Review of attachment 290835 [details] [review]: Looks good.
Pushed
This isn't fixed.
(In reply to comment #8) > This isn't fixed. Are you still able to reproduce it?
The: while(line[j] == ' ') j++ code is still problematic.
Right, though it is not causing the crash. But could be a problem if in the future the kernel just change the format of that file.
I think it best to either fix it, or make sure the bug is documented in the source if we ever hit it again.
Created attachment 291659 [details] [review] dleyna: Handle broken /proc/net/tcp file Don't crash if we reach the end of the line when skipping the first field of /proc/net/tcp
Review of attachment 291659 [details] [review]: This is done for is_our_user_ipv4(), but we need to do it also in is_our_user_ipv6()
Created attachment 291668 [details] [review] dleyna: Handle broken /proc/net/tcp file Don't crash if we reach the end of the line when skipping the first field of /proc/net/tcp
Attachment 291668 [details] pushed as 465c58c - dleyna: Handle broken /proc/net/tcp file