On Thu, 9 Feb 2017 09:57:29 -0600 Derek Foreman <[email protected]> said:
> On 08/02/17 06:34 PM, Carsten Haitzler (The Rasterman) wrote: > > On Wed, 08 Feb 2017 16:00:06 +0000 Mike Blumenkrantz > > <[email protected]> said: > > > >> Hi, > >> > >> We've had threads and discussions about blindly pushing fixes for Coverity > >> issues before, and it seemed to me that the consensus was we should stop > > > > this wasn't blind. see below. > > > >> engaging in such risky behaviors and only focus on issues in areas that we > >> directly work on and are actively testing. It's easy enough to make > > > > this i consider a bigger issue as its a security issue for setuid apps. it's > > actually rather critical. thus i consider the importance of fixing such > > things high up the priority list. there is yet more to fix still... > > > >> mistakes while contributing ordinary untested code, but it seems to be a > >> pattern that more mistakes are made when contributing untested code in > >> unfamiliar areas. > >> > >> Let's try to work more collaboratively on these and notify the people who > >> work on the corresponding components when we're working on fixes for them. > > > > i was in general hunting for getenv's for XDG_RUNTIME_DIR because this is > > one of those env vars that's unsafe to use in a setuid app. coverity > > happened to be pointing at this too (it was after coverity pointed to a few > > other places that i started grep -r XDG_RUNTIME_DIR in our code). if > > anything relies on it in a setuid app then it's in for a huge amount of > > trouble. there are still more places like this too in our code i have to > > hunt. > > > > that should not have broken though... it gets xdg_runtime from vpath... and > > that should be using the env var UNLESS it's a setuid app and THEN its > > totally unsafe to use the env var... > > > > i read the code - the ONLY place in elput that dealt with that tmp file is > > right there... so it SHOULD have made zero difference even where it was > > placed as that filename is a tmpfile thus no code outside of that function > > should even know what it is (it's not as "well known named file"), and the > > only thing it exposes is an fd - so it could use memfd and not even a real > > file... i read the code before and saw no good reason this shouldn't work > > just fine. "collaborating" would have done nothing to help. there already > > is notification > > - git commits list. it's automated. > > > > regardless the code is now back to "setuid unsafe". so never ever ever use > > elput in any setuid binary. > > > > so i get that you are annoyed, but maybe just read git commits list? cedric > > pushed/approved a patch that dropped everything in sw engine in evas to > > like 5 to 10fps yesterday. should he have notified everyone about his > > change to evas and "collaborated"? > > FWIW I noticed this right at the end of my work day and was planning on > investigating further the next day - your revert obviously saved me some > time. Thanks! :D we all have to do this. it's how things work. :) > > git commits list is there for notification of things being done to various > > components. if its a big and/or extensive and controversial change - yeah. > > discuss. this is not that. it's changing the source of an env var in a few > > lines of code in a single commit. > > I start every morning with a quick look at new commits - I have to do > this so I can see at a glance if anything is likely to have destroyed > wayland support again. > > Unfortunately sometimes the breakage is non-obvious - especially when a > change like the one cedric approved isn't clearly related, or last > night's "vpath usage" commit touches the world. > > Chasing down these non-obvious breaks is time consuming, and takes time > away from other development. Time that would be saved if the commits > were adequately tested before landing. sure. i've done this 100's of times over the years. often it's "WFM" for someone and someone else sees the break. i didnt notice the keyboard not working - i did actually run e in wl mode... i just didnt actually type anything so didn't notice. but things seemed to work. it's how it works. i have woken up and done a git pull/rebase and rebuild and found my desktop hosed as some commit overnight broke e to the point of non-functioning. i was stuck in a text console... but i hunt down the issue and fix it. sometimes my patience only goes as far as a bisect then revert sometimes i actually find the real problem and fix it... depends how annoyed i am. :) but git is out communications medium. it's where we share what we have done with others and if it has issues the author didn't see, hopefully others do and catch it. > > the issue is i just forgot the / after the vpath string. just the / - > > everything else was just fine. testing is hard as the only test box i have > > at the office is my old gen 1 i5 and that is a "wait an hour for it to sync > > files over and build" and i thought what i did was low risk as it was > > correct. it was wrong. by one character. > > > >> On Wed, Feb 8, 2017 at 10:50 AM Derek Foreman <[email protected]> > >> wrote: > >> > >>> derekf pushed a commit to branch master. > >>> > >>> > >>> http://git.enlightenment.org/core/efl.git/commit/?id=62a22fd401128affb4f9bdca6d6db35a90dd6e19 > >>> > >>> commit 62a22fd401128affb4f9bdca6d6db35a90dd6e19 > >>> Author: Derek Foreman <[email protected]> > >>> Date: Wed Feb 8 09:37:49 2017 -0600 > >>> > >>> Revert "elput - use vpath to get xdg runtime to also be setuid safe" > >>> > >>> This reverts commit 24e34e19a1db84cdcb6241207cb99d14ca83c41b. > >>> > >>> This broke keyboard input for the enlightenment wayland compositor, > >>> please test elput changes on at least one of the drm backends, > >>> preferably with enlightenment. > >>> > >>> The wayland compositor is hard enough to keep stable due to breakage > >>> from core changes only tested on X - but elput's main user is our > >>> wayland compositor, was this tested anywhere? > >>> --- > >>> src/lib/elput/elput_evdev.c | 15 ++++++--------- > >>> 1 file changed, 6 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/src/lib/elput/elput_evdev.c b/src/lib/elput/elput_evdev.c > >>> index 028bf62..a83ebc1 100644 > >>> --- a/src/lib/elput/elput_evdev.c > >>> +++ b/src/lib/elput/elput_evdev.c > >>> @@ -60,19 +60,16 @@ _keyboard_modifiers_update(Elput_Keyboard *kbd, > >>> Elput_Seat *seat) > >>> static int > >>> _keyboard_fd_get(off_t size) > >>> { > >>> - const char *path; > >>> - Eina_Tmpstr *fullname; > >>> - long flags; > >>> int fd = 0; > >>> + char *path; > >>> char tmp[PATH_MAX]; > >>> + long flags; > >>> + Eina_Tmpstr *fullname; > >>> + > >>> + if (!(path = getenv("XDG_RUNTIME_DIR"))) > >>> + return -1; > >>> > >>> - Efl_Vpath_File *file_obj = > >>> - efl_vpath_manager_fetch(EFL_VPATH_MANAGER_CLASS, "(:run:)"); > >>> - efl_vpath_file_do(file_obj); > >>> - efl_vpath_file_wait(file_obj); > >>> - path = efl_vpath_file_result_get(file_obj); > >>> snprintf(tmp, sizeof(tmp), "%s/elput-keymap-XXXXXX", path); > >>> - efl_del(file_obj); > >>> > >>> fd = eina_file_mkstemp(tmp, &fullname); > >>> if (fd < 0) return -1; > >>> > >>> -- > >>> > >>> > >>> > >> ------------------------------------------------------------------------------ > >> Check out the vibrant tech community on one of the world's most > >> engaging tech sites, SlashDot.org! http://sdm.link/slashdot > >> _______________________________________________ > >> enlightenment-devel mailing list > >> [email protected] > >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > >> > > > > > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) [email protected] ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
