GNOME Bugzilla – Bug 680215
Don't silently accept extra arguments to C functions
Last modified: 2017-04-04 01:41:27 UTC
This is a bug waiting to happen, or in some cases, a bug that already exists. I have patches to make gnome-shell "work" again, but there may be other, unspotted scenarios.
Created attachment 219175 [details] [review] GLib: Don't pass extra arguments to constructors
Created attachment 219176 [details] [review] function: Reject cases where we pass too many arguments We shouldn't be ignoring arguments that the user passed to a function. This is just a bug waiting to happen.
The standard JavaScript language lets you do this; I assume that was the rationale for allowing it. Maybe there's an argument for a new GJS_LINT=1 environment variable or something, but this would be pretty gratutious breakage for people.
(In reply to comment #3) > The standard JavaScript language lets you do this; I assume that was the > rationale for allowing it. JavaScript also lets you pass fewer or no arguments at all, and they will automatically be filled in with 'undefined's.
(In reply to comment #4) > (In reply to comment #3) > > The standard JavaScript language lets you do this; I assume that was the > > rationale for allowing it. > > JavaScript also lets you pass fewer or no arguments at all, and they will > automatically be filled in with 'undefined's. True; but we couldn't mimic that behavior really without triggering g_return_if_fail()s at a minimum, an in a lot of cases segfault. Clearly not the same thing as silently doing nothing with extra arguments. I'm not going to say that it's better to ignore arguments though; the point is more that it'd be *really painful* to switch to throwing an error now.
I think there would be a burst of initial pain, and then a lot of relief. And the overrides patch can certainly go in.
Created attachment 220589 [details] [review] function: Correct argument counting for array arguments For an (out) array length argument, we were accidentally subtracting it from the expected argc.
Review of attachment 219175 [details] [review]: Ok.
Review of attachment 220589 [details] [review]: The commit message is wrong; we *should* subtract an (out) length from the expected js argc, but we weren't doing it, correct?
(In reply to comment #6) > I think there would be a burst of initial pain, and then a lot of relief. So...doing this this late in the 3.6 cycle seems very dangerous to me. How about instead a transition period where we just g_printerr() or something?
Review of attachment 220589 [details] [review]: No; we should never subtract an (out) parameter from the number of parameters that need to be passed into a C function, length or not.
Review of attachment 220589 [details] [review]: Yeah, sorry, I read the patch backwards.
Attachment 219175 [details] pushed as ce0a129 - GLib: Don't pass extra arguments to constructors Attachment 220589 [details] pushed as 4d31cc9 - function: Correct argument counting for array arguments
I think we should do this for 3.8. I've removed all the issues with gnome-shell, gnome-documents, and sushi over the course of time. I've been running with this patch since a little before I filed it, and I haven't encountered any issues since.
Review of attachment 219176 [details] [review]: Going to take Colin's approach for now, of warning rather than throwing. Once that's been in for a while, then we can consider doing something like this.
Created attachment 349157 [details] [review] function: Warn when passing extra arguments to a function We should not silently ignore arguments that a user passes to a GI function. Instead, print a warning. Silently ignoring them can lead to hard-to-track-down bugs.
Review of attachment 349157 [details] [review]: Thanks, looks good.
Pushed with one small amendment: JS_ReportWarning() instead of g_warning() so that we get a file and line number in the message. Attachment 349157 [details] pushed as ab0c97b - function: Warn when passing extra arguments to a function