GNOME Bugzilla – Bug 775913
subprocesslauncher: potential infinite loop in verify_disposition()
Last modified: 2017-01-03 23:53:16 UTC
Created attachment 341698 [details] [review] use twos-compliment to disable filtered_flags after checking them When building error strings in verify_disposition(), the flags that are set are walked. However, instead of disabling the filtered_flags bit after checking it, the bit is set (overwriting the already set bit).
Created attachment 341699 [details] [review] fixes the two potential infinite loops in verify_disposition() I found another infinite loop potential in this error handling path. It is also possible to get a valid GFlagsValue when filtered_flags is zero G_SUBPROCESS_FLAGS_NONE). So we will also infinitely loop on that since there is no bit to disable.
Review of attachment 341699 [details] [review]: This looks right to me, but I almost wonder if it would be clearer to use a for loop over the flags from the class struct rather than the accessor api. ::: gio/gsubprocesslauncher.c @@ +84,2 @@ class = g_type_class_peek (G_TYPE_SUBPROCESS_FLAGS); + while (filtered_flags != G_SUBPROCESS_FLAGS_NONE && by putting this check here, it almost makes me think filtered_flags could be NONE before the loop starts, which isn't the case. I think it would be clearer (personally, don't know if you or others agree) if you did if (!filtered_flags) break; or something at the bottom of the loop. @@ +84,3 @@ class = g_type_class_peek (G_TYPE_SUBPROCESS_FLAGS); + while (filtered_flags != G_SUBPROCESS_FLAGS_NONE && + NULL != (value = g_flags_get_first_value (class, filtered_flags))) I know this is defensive programming to put the literal on the left, but I don't think glib follows that style right now so it looks a little out of place.
Created attachment 341921 [details] [review] subprocess: avoid infinite loop in verify_disposition() When performing the verify and building the error string there were two possibilities of an infinite loop. The first is the missing twos-complement to unset the bit in the filtered flags. The second is the lack of handling G_SUBPROCESS_FLAGS_NONE which can return a valid GFlagsValue (and cannot unset the bit since the value is zero). Instead, to be safer we handle NONE as a special case. Alternatively this will walk all known values in the GSubprocessFlags type class and check if they are set. This has the benefit that we don't call needless functions which walk the same table as well as avoiding mutating values to build the error string.
Review of attachment 341921 [details] [review]: this looks much prettier to me ::: gio/gsubprocesslauncher.c @@ +86,3 @@ + if (filtered_flags == G_SUBPROCESS_FLAGS_NONE) + { + g_string_append (err, " none"); actually, I don't think you want to print the none case. If flags are none, then they aren't part of the problem getting highlighted.
Created attachment 341981 [details] [review] subprocess: avoid infinite loop in verify_disposition() When performing the verify and building the error string there were two possibilities of an infinite loop. The first is the missing twos-complement to unset the bit in the filtered flags. The second is the lack of handling G_SUBPROCESS_FLAGS_NONE which can return a valid GFlagsValue (and cannot unset the bit since the value is zero). Instead, to be safer we handle NONE as a special case. Alternatively this will walk all known values in the GSubprocessFlags type class and check if they are set. This has the benefit that we don't call needless functions which walk the same table as well as avoiding mutating values to build the error string.
Review of attachment 341981 [details] [review]: looks right to me! (sorry for forgetting about this, I was just reading through that file for unrelated reasons and remembered that I dropped this bug on the floor)
actually "Instead, to be safer we handle NONE as a special case." probably shouldn't be in the commit message.
Comment on attachment 341981 [details] [review] subprocess: avoid infinite loop in verify_disposition() Pushed with updated commit message.