GNOME Bugzilla – Bug 738277
g_output_stream_flush_async() can left stream in a fake pending state
Last modified: 2018-02-13 14:06:04 UTC
While checking when "Stream has outstanding operation" error can happened I noticed that g_output_stream_flush_async() can left the output stream in a fake pending state, when the subclass doesn't implement flush_async() virtual method. Whether this can really happen I do not know, I only looked for it due to the error being reported for evolution at bug #737733, but that doesn't make much sense to me, because, according to the log, evolution does things right. I asked for further investigation on the reporter's side to know more, but meanwhile this bug is in GLib regardless.
Good catch. I don't think there's any reason to overcomplicate this issue; let's just swap the order of the conditionals and move on.
Actually, it's implemented by GOutputStream itself. Now, flush (the sync version of the vfunc) might be unimplemented. I can reproduce the problem you describe if I make this change to the code: - if (class->flush_async == NULL) + if (class->flush == NULL) But flush_async will never be unimplemented. That's dead code. You're correct that the check is in the wrong place, but I guess it makes more sense to remove it than to reorder it.
Created attachment 368202 [details] [review] outputstream: Fix check inside flush_async flush_async is implemented by GOutputStream itself, so this check is currently dead code. And if it were reachable, then it would leave the GOutputStream in an invalid state. The stream has already been marked as pending, but now the flag would never be cleared, and all subsequent operations will fail. I almost removed this condition, which would have been fine. But instead, I suppose it makes sense to check here if flush is implemented instead. There's no point in starting a thread to run flush_async if it's just going to do nothing and return TRUE because flush is unimplemented. This is just a minor optimization. Now, in theory, this could break any GOutputStream that overrides flush_async but not flush. So, arguably, it might be more correct to just remove the condition altogether. Hopefully no applications do that; if so, this condition will have to be removed instead.
Review of attachment 368202 [details] [review]: Actually, the semantics of this are wrong. flush_async should not succeed immediately if there is a pending operation. Bad idea. Let's remove the condition instead.
Created attachment 368203 [details] [review] outputstream: Remove incorrect check inside flush_async This check is problematic for multiple reasons: (1) flush_async is implemented by GOutputStream itself, so the check will never fail. It's dead code. (2) If this branch were ever entered, the GOutputStream would be left in an invalid state. The pending flag would be set, and not unset, causing subsequent operations to fail. I considered switching the order of these checks and looking to see if flush is implemented instead, as an optimization. But the semantics of that would be weird, since flush_async would then immediately return successfully even if nothing has been flushed. Seems best to just remove this condition.
Review of attachment 368203 [details] [review]: > (1) flush_async is implemented by GOutputStream itself, so the check > will never fail. It's dead code. Not strictly true. A class which derives from GOutputStream could set `G_OUTPUT_STREAM_GET_CLASS(me)->flush_async = NULL`. Could you not just call g_output_stream_clear_pending() in the `class->flush_async == NULL` branch, before returning? There might be other similar code paths which need fixing in this class, or other stream classes which support pending operations (GIOStream, GInputStream).
(In reply to Philip Withnall from comment #6) > Could you not just call g_output_stream_clear_pending() in the > `class->flush_async == NULL` branch, before returning? Could just flip the conditionals around. Let's do that. (In reply to Philip Withnall from comment #6) > Not strictly true. A class which derives from GOutputStream could set > `G_OUTPUT_STREAM_GET_CLASS(me)->flush_async = NULL`. OK, I didn't know GObject worked this way. None of the other vfuncs besides flush are optional in this manner, but OK, I understand that if the vfunc could be deleted in the past, code might rely on that, and it should remain possible to delete in the future.
(In reply to Michael Catanzaro from comment #7) > (In reply to Philip Withnall from comment #6) > > Could you not just call g_output_stream_clear_pending() in the > > `class->flush_async == NULL` branch, before returning? > > Could just flip the conditionals around. Let's do that. Nope, because that would affect the error handling done on our behalf in g_output_stream_set_pending(): calling g_output_stream_flush_async() on a closed output stream would no longer return an error. > None of the other vfuncs besides flush are optional in this manner, but OK, > I understand that if the vfunc could be deleted in the past, code might rely > on that, and it should remain possible to delete in the future. Thanks for checking.
(In reply to Philip Withnall from comment #8) > Nope, because that would affect the error handling done on our behalf in > g_output_stream_set_pending(): calling g_output_stream_flush_async() on a > closed output stream would no longer return an error. You're right. Touching glib is scary....
Created attachment 368282 [details] [review] goutputstream: Fix missing call to clear_pending in flush_async If flush_async is deleted by a child class, then calling g_output_stream_flush_async would leave the GOutputStream in an invalid state. I'm not aware of any GOutputStream that would be affected by this issue, but might as well fix it.
Review of attachment 368282 [details] [review]: ++ (In reply to Michael Catanzaro from comment #9) > Touching glib is scary.... Don’t be silly, it’s just code. It behaves as deterministically as any other bit of C. :-)
Attachment 368282 [details] pushed as c3c7b52 - goutputstream: Fix missing call to clear_pending in flush_async