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 > 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. > 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
