GNOME Bugzilla – Bug 711048
glocalfileinputstream.c allows skip past end of file
Last modified: 2013-11-03 04:59:09 UTC
The efficient skip implementation of glocalfileinputstream.c allows skipping past the end of the file (causing subsequent calls of g_seekable_tell() to give the wrong value) which is incorrect with respect to the documentation: """ This is identical to g_input_stream_read(), from a behaviour standpoint, but the bytes that are skipped are not returned to the user. Some streams have an implementation that is more efficient than reading the data. """
Created attachment 258406 [details] [review] gio: Don't allow skipping past the end of GLocalFileInputStream The overridden implementation of the skip method for GLocalFileInputStream allows skipping past the end of the file which is inconsistent with the documentation. Prevent this by first seeking to the end of the file and then seeking backwards from there as much as is necessary.
Review of attachment 258406 [details] [review]: Ok, a few things: 1) Can you briefly mention the real-world application if any that caused you to find this? That type of thing can be really useful when tracking down issues later. Something like "The gvfs mime code uses skip, and hit this error..." (just an offhand guess). 2) I'm always a lot more confident reviewing patches like this if they come with tests. 3) Do we even need this code at all? I am confused why we need to both implement GInputStream->skip() and GSeekable. The base GInputStream skip will just call into the GSeekable implementation for GLocalFileInputStream. So would it also work to just delete g_local_file_input_stream_skip(), or am I crazy?
(In reply to comment #2) > 3) Do we even need this code at all? I am confused why we need to both > implement GInputStream->skip() and GSeekable. The base GInputStream skip will > just call into the GSeekable implementation for GLocalFileInputStream. So > would it also work to just delete g_local_file_input_stream_skip(), or am I > crazy? in theory we don't need this implementation. see bug 681374. However, the patch there is awkward because it runs into the problem of recursive g_input_stream_set_pending() calls (eg, g_file_input_stream_seek() calls g_input_stream_set_pending(), so it can't be called from inside g_input_stream_skip()). (That problem is also discussed in bug 707912.) So this patch may be a good temporary fix...
(In reply to comment #2) > Review of attachment 258406 [details] [review]: > > Ok, a few things: > > 1) Can you briefly mention the real-world application if any that caused you to > find this? That type of thing can be really useful when tracking down issues > later. > Something like "The gvfs mime code uses skip, and hit this error..." (just an > offhand guess). Unfortunately not that interesting. I'm writing a test suite for gio and the gvfs implementations of it (not published anywhere yet...) and fixing or implementing things as I find them. Mostly the local file implementation is correct according to the documentation and the gvfs backends have weird corner cases but in this case it was reversed... Yes, it is a corner case but either it should be fixed or the docs should be changed :-) > > 2) I'm always a lot more confident reviewing patches like this if they come > with tests. I haven't looked at the glib test suite before but let me see if I can put something together. > > 3) Do we even need this code at all? I am confused why we need to both > implement GInputStream->skip() and GSeekable. The base GInputStream skip will > just call into the GSeekable implementation for GLocalFileInputStream. So > would it also work to just delete g_local_file_input_stream_skip(), or am I > crazy? But doesn't that still allow you to skip past the end of the file if it just defers to seek? I guess it would work with Dan's patch in #681374. (In reply to comment #3) > (In reply to comment #2) > > 3) Do we even need this code at all? I am confused why we need to both > > implement GInputStream->skip() and GSeekable. The base GInputStream skip will > > just call into the GSeekable implementation for GLocalFileInputStream. So > > would it also work to just delete g_local_file_input_stream_skip(), or am I > > crazy? > > in theory we don't need this implementation. see bug 681374. However, the patch > there is awkward because it runs into the problem of recursive > g_input_stream_set_pending() calls (eg, g_file_input_stream_seek() calls > g_input_stream_set_pending(), so it can't be called from inside > g_input_stream_skip()). (That problem is also discussed in bug 707912.) So this > patch may be a good temporary fix... Ah, I didn't see that bug. But yeah, this fix should be good for the meantime.
(In reply to comment #4) > > Unfortunately not that interesting. I'm writing a test suite for gio and the > gvfs implementations of it (not published anywhere yet...) glib/gio/tests *is* a test suite for gio (and gvfs to some degree). > I guess it would work with Dan's patch in #681374. Ok, I'm going to defer to Dan on this one. I am not familiar at all with the gio seeking code. If you feel confident enough to review his patch, please do!
Created attachment 258478 [details] [review] gio: Don't allow skipping past the end of GLocalFileInputStream The overridden implementation of the skip method for GLocalFileInputStream allows skipping past the end of the file which is inconsistent with the documentation. Prevent this by first seeking to the end of the file and then seeking backwards from there as much as is necessary.
The updated patch adds a test. (In reply to comment #5) > (In reply to comment #4) > > > > Unfortunately not that interesting. I'm writing a test suite for gio and the > > gvfs implementations of it (not published anywhere yet...) > > glib/gio/tests *is* a test suite for gio (and gvfs to some degree). Yeah, I'm just doing it to familiarize myself with the whole API and where the local backend and the gvfs backends stand in terms of functionality. > > > I guess it would work with Dan's patch in #681374. > > Ok, I'm going to defer to Dan on this one. I am not familiar at all with the > gio seeking code. If you feel confident enough to review his patch, please do! Nope :-)
Comment on attachment 258478 [details] [review] gio: Don't allow skipping past the end of GLocalFileInputStream >+ if (res - start > count) >+ { >+ res = lseek (file->priv->fd, count - (res - start), SEEK_CUR); I think it would be more obvious as: res = lseek (file->priv->fd, start + count, SEEK_START); Also, the function would probably be clearer now if you renamed "res" to "end". test case looks good. Feel free to commit with the above change.
Pushed to master as 89f9615. Thanks!