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