So I looked at this function because we were emitting a warning due to ignoring the return value of write(). This turned into some yak shaving around error handling: 1) We were only checking WEXITSTATUS(), but this is broken in the case where !WIFEXITED(), for example WIFSIGNALED(). If available, use g_spawn_check_exit_status() from newer GLib which makes this easy. 2) If we failed to read the loginuid, we would happily pass uninitialized data from the stack and write that as our login uid. Handle this by only doing the loginuid dance if we read something. Also, use G_N_ELEMENTS() instead of hardcoding the length again.
Created attachment 67350 [details] [review] util: Clean up spawn_with_login_uid() error handling
Comment on attachment 67350 [details] [review] util: Clean up spawn_with_login_uid() error handling Review of attachment 67350 [details] [review]: ----------------------------------------------------------------- a few comments: 1) pretty sure kernel doesn't let us spoof loginuid anymore, so we probably should just stop trying 2) get_caller_loginuid never fails, it will always initialize loginuid to something (root if nothing else). ::: src/util.c @@ +223,5 @@ > + WEXITSTATUS(estatus)); > + return FALSE; > + } > + return TRUE; > +#endif I don't like this #if stuff. pick one half and go with it, let's not have code paths that rot away. @@ +237,5 @@ > + if (fd == -1) > + _exit (1); > + if (write (fd, id, strlen (id)) == -1) > + _exit (1); > + (void) close (fd); don't like casting return values to (void). It makes me think of stevens or something. close() isn't warn_unused_result afaik, so don't bother. Also, it's not okay to fail if any of this fails, since all of this will always fail, iirc. @@ +254,2 @@ > > + loginuid[0] = '\0'; this is unnecessary @@ +257,2 @@ > > + if (loginuid[0]) { this will always happen. @@ +257,5 @@ > > + if (loginuid[0]) { > + setup_func = setup_loginuid; > + setup_data = loginuid; > + } else { this will never happen.
(In reply to comment #2) > 1) pretty sure kernel doesn't let us spoof loginuid anymore, so we probably > should just stop trying That's a build-time option "CONFIG_AUDIT_LOGINUID_IMMUTABLE" that isn't set in the Fedora 17 kernel at last. > 2) get_caller_loginuid never fails, it will always initialize loginuid to > something (root if nothing else). Ah, true. > I don't like this #if stuff. pick one half and go with it, let's not have code > paths that rot away. Well, do you want a GLib 2.34 hard dependency? > don't like casting return values to (void). It makes me think of stevens or > something. Mmm...I think reducing spew from automated code analysis tools is very important - casting results to (void) is "The Standard" way to note that we're ignoring a return value. For C projects of substantial size, at some point you need to turn to static analysis. > close() isn't warn_unused_result afaik, so don't bother. True at present, but in situations where you're writing to a filesystem, you ignore the result of close() at your peril - otherwise you're very likely to miss that ENOSPC.
(In reply to comment #3) > (In reply to comment #2) > > > 1) pretty sure kernel doesn't let us spoof loginuid anymore, so we probably > > should just stop trying > > That's a build-time option "CONFIG_AUDIT_LOGINUID_IMMUTABLE" that isn't set in > the Fedora 17 kernel at least. I assume that's a "bandwagon" option that all new distros turn on, and old distros with new kernels will toggle off just to maintain compatibility. I'd say lets drop it from new account service and add it back if there are complaints (which I doubt there will be). > Well, do you want a GLib 2.34 hard dependency? Probably not, so I'd say let's go with the #else clause for now, and add the new code later. We can leave the bug open so we don't forget. > > don't like casting return values to (void). It makes me think of stevens or > > something. > > Mmm...I think reducing spew from automated code analysis tools is very > important - casting results to (void) is "The Standard" way to note that we're > ignoring a return value. I'd rather get the return value and check it than put a (void). > For C projects of substantial size, at some point you need to turn to static > analysis. > > > close() isn't warn_unused_result afaik, so don't bother. > > True at present, but in situations where you're writing to a filesystem, you > ignore the result of close() at your peril - otherwise you're very likely to > miss that ENOSPC. Sure, there are two options in my mind 1) ignore the value, don't put (void) 2) don't ignore the value, check it. There are cases when 1) is appropriate and there are cases when 2) is appropriate, but i don't like 3) ignore the value, put (void) since it's ugly and shouldn't be necessary. You're saying you're using a static analysis tool that's more pedantic about return values and doesn't take __attribute__((warn_unused_result)) into account?
Created attachment 67406 [details] [review] Only clean up WIFEXITED This patch only addresses the WIFEXITED() thing. I kept in the GLIB_CHECK_VERSION bit...versus the alternative of only remembering 5 years later that this code has a GLib replacement, it seems better.
(In reply to comment #4) > since it's ugly and shouldn't be necessary. You're saying you're using a > static analysis tool that's more pedantic about return values and doesn't take > __attribute__((warn_unused_result)) into account? The static analysis tool I'm thinking of looks for *consistency*. If you check a return value in one place, you're expected to check it everywhere.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.