Re: Friday is for fun: fixing the error paths in selinux
Hi Lukas, everyone,toggle quoted messageShow quoted text
this looks fun indeed, so I gave it a try yesterday night.
Nice challenge, it is shaping up to be a good excample for why stuff is not that simple and streightforward to trace, even with Git at your disposal, but I guess you knew that already ^^
Not sure if got to the bottom of this mystery, but maybe something can be learned from the path I took to get there and from what more experienced Git users have to say about it.
What I did and what I learned:
1) git blame -L 2035,2045 -l services.c
- Gives us the revision where the dead stores were introduced in the first place
Author is Eric Paris <eparis@...>
- The log entry states, this commit is to unify the design pattern of the error paths to the following pattern:
- So the idea was to first set the errorcode rc, and jump to "out:" in case of a failure, where the errorcode rc would be returned (Possibly another example of the one return per function only design paradigm)
- Nevetheless in that commit the dead stores are already introduced in all instances where the control jumps to the label "bad:" instead of "out:"
Quite possibly on purpose though to have the same pattern everywhere and possibly to protect against introduction of future bugs popping up when the code might be modified to jump to "out:" instead.
2) I assume, the code was reworked at a later date doing away with the pattern Eric used, so I looked at the revision removing the label "out:", after all it is not in the most recent version of code anymore
git blame --reverse 4b02b524487622ce1cf472123899520b583f47dc.. -l services.c
- That way I find the last revision still having the line 7d0250ed8e69fb6a66caecf60b8753a21224cc1a which means the next revision 23d19ba06b9c5614d6457f5fed349ec8f6d4dac9 is the one making the change.
3) Turns out this is a merge revision merging a branch, that itself came from Linux 5.0-rc1 with hash bfeffd155283772bbe78c6a05dec7c0128ee500c
4) Another Blame like 1) to find where that version comes from leads right back to Eric commit above, so it found it's way into Linux 5.0-rc1 as well at some point.
- This means the messy situation basically boils down to having the same patch integrated in two parallel branches that then are merged at some point discarding the change, which leads to blame leading us down the wrong branch so to speak.
5) This also means the changes must have happend somehwere between Erics commit and the head of Linux 5.0-rc1.
-To get the chain of commits between Erics commit and the release candidate 5.0-rc1 I tried to use the --ancestry path option of git rev-list
git rev-list --ancestry-path 4b02b524487622ce1cf472123899520b583f47dc..bfeffd155283772bbe78c6a05dec7c0128ee500c
- Maybe I misunderstand how this command works, but according to the output, there are 576463 commits on the path between the Erics Commit and the head of 5.0-rc1 ... so thats a dead end.
6) One more attempt with a different vector of attack, I set a view in gitk to filter all commits making changes to services.c and sure enough a "manual" bisection yields the revision removing the "output:" label
ee1a84fdfeedfd7362e9a8a8f15fedc3482ade2d by Ondrej Mosnacek <omosnace@...> 2018-11-30 16:24:08
- This seems to be a functional change of how stuff works under the hood, and the removal of the "output:" label is not adressed in the log entry, seems to be a byproduct of reworking the function.
- In that revision all instances where before there would be a jump to "out:" are replaced by direct returns, and the preceding setting of the variable "rc" is removed, the other instances however are not touched and remain in there.
Most likely a remnant leftover from Ondrej changing the defensive programming design pattern Eric introduced a decade earlier.
What do you think?
Mit freundlichen Grüßen
Dr. rer. nat. Jochen Kall
ITK Engineering GmbH
Im Speyerer Tal 6
Tel.: +49 7272 7703-546
Fax: +49 7272 7703-100
mailto:jochen.kall@... ( jochen.kall@... )
ITK Engineering GmbH | Im Speyerer Tal 6 | 76761 Rülzheim
Tel.: +49 7272 7703-0 | Fax: +49 7272 7703-100
mailto:info@... ( info@... ) | http://www.itk-engineering.de ( http://www.itk-engineering.de/ )
Vorsitzender des Aufsichtsrats/Chairman of the Supervisory Board:
Dr. Rudolf Maier
Michael Englert (Vorsitzender/Chairman), Bernd Gohlicke
Sitz der Gesellschaft/Registered Office: 76761 Rülzheim
Registergericht/Registered Court: Amtsgericht Landau, HRB 32046
USt.-ID-Nr./VAT-ID-No. DE 813165046