On Wed, 23 Jan 2008 05:11:33 -0800 "laurent FANIS" <[EMAIL PROTECTED]> babbled:
> Hi, > > A long over due mail with the OpenBSD specific patches inlined. > The files can be checked directly from > http://www.openbsd.org/cgi-bin/cvsweb/ports/x11/e17/ > This in the -current system (http://www.openbsd.org/faq/faq5.html#Flavors). > This is based on a snapshot from 20071211. do you do the ports patching? if so you've done a great job at breaking e in openbsd ports. > In eet: > > --- src/lib/eet_lib.c.orig Mon Nov 12 11:14:04 2007 > +++ src/lib/eet_lib.c Mon Nov 12 11:15:11 2007 > @@ -497,6 +497,10 @@ eet_open(const char *file, Eet_File_Mode mode) > else > return NULL; > } > + else if (file_stat.st_size == 0) > + { > + return NULL; > + } > > /* We found one */ > if (ef && (file_stat.st_mtime != ef->mtime)) old - eet is even pickier. it requires the file be > 3 ints of size (which in 99.9999% of cases int is 32bit - yes. technically it might not be - but if you are on a system where it is not - 99.9999% of your software just broke anyway :) ). not needed. > > Warnings: > Four warnings:about "HAVE_STDLIB_H" being redefined. i don't know where this comes from, but config.h is included onle once in Eet_private.h and that is included only once in each .c file. where are the previous definitions? > Two warning about unsafe function : strcpy and sprintf instead of > strlcpy and snprintf where? you might find that these are safe anyway as the pre-conditions make them safe. if we missed them - telling us will be useful :). i can go through all of them: eet_lib.c: check the code above the strcpy. the buffer is alloced to be bigger than the string being strcpy'ed in. it's safe. eet_data.c: 1st instance: safe. malloced buffer of correct size exists to strcpy into. 2nd instance: same as above 3rd instance: same as above 4th instance: commented out 5th instance: commented out 6th instance: commented out sprintf: buffers used are 64 bytes. this is more than large enough to hold any possible output from _eet_double_to_string_convert() and the null terminator. format is (up to a max): 0xX. (4 bytes - X is 1 hex value) XXXXXXXXXXXXXXXX (16 bytes - hex) pX (2 bytes) XXXX (exponent in decimal max value with a max sized double will be 1023 - so 4 bytes) so a total buffer size requirement is 4 + 16 + 2 + 4 = 26 bytes, plus nul byte terminator, so 27. we have 64. we're safe as houses. :) > In Evas: > > $OpenBSD: patch-configure_in,v 1.1 2007/12/06 19:47:08 bernd Exp $ > --- configure.in.orig Wed Dec 5 22:13:29 2007 > +++ configure.in Wed Dec 5 22:13:42 2007 > @@ -31,7 +31,7 @@ AC_SUBST(version_info) > > AC_FUNC_ALLOCA > > -MODULE_ARCH="$host_os-$host_cpu" > +MODULE_ARCH="" > AC_SUBST(MODULE_ARCH) > AC_DEFINE_UNQUOTED(MODULE_ARCH, "$MODULE_ARCH", "Module architecture") you just broke multi-arch support if you install modules to ~/.evas/modules (imagine your homedir is rsync'ed or nfs shared between a sparc solaris box, an x86 solairs box, a openbsd x86 box, an x86-64 linux box. each module will have its binaries in a different arch dir - and will work - as long as you built it for that arch+os. this patch summarily breaks that. > $OpenBSD: patch-src_lib_include_evas_common_h,v 1.1 2007/12/07 > 09:49:58 mbalmer Exp $ > --- src/lib/include/evas_common.h.orig Fri Dec 7 10:34:46 2007 > +++ src/lib/include/evas_common.h Fri Dec 7 10:34:59 2007 > @@ -46,6 +46,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > +#include <stdint.h> > #include <string.h> > #include <signal.h> > #include <setjmp.h> what problems did you fix by doing this? we rarely use uint32/64 etc. etc. (and friends). i'll put this in as it should be harmless unless your os is in a retirement home awaiting its final days. > Warnings: > evas_convert_rgb_8.c: In function > `evas_common_convert_rgba_to_8bpp_rgb_221_dith': > evas_convert_rgb_8.c:162: warning: right shift count is negative > evas_convert_rgb_8.c: In function > `evas_common_convert_rgba_to_8bpp_rgb_121_dith': > evas_convert_rgb_8.c:195: warning: right shift count is negative > evas_convert_rgb_8.c: In function > `evas_common_convert_rgba_to_8bpp_rgb_111_dith': > evas_convert_rgb_8.c:228: warning: right shift count is negative not important. limits it to 0 for us. that's the intended effect. > and: > evas_outbuf.c: In function `evas_buffer_outbuf_buf_push_updated_region': > evas_outbuf.c:268: warning: assignment from incompatible pointer type > evas_outbuf.c:284: warning: assignment from incompatible pointer type > evas_outbuf.c:310: warning: assignment from incompatible pointer type > evas_outbuf.c:341: warning: assignment from incompatible pointer type unfortunately no directory for this file - so no way to know which one of several evas_outbuf.c's it might be. chances are these are entirely harmless and all we will do is shut the compiler up with some casts - which it does anyway. > And: > evas_engine.c: In function `eng_font_draw': > evas_engine.c:855: warning: passing arg 3 of > `evas_common_draw_context_font_ext_set' from incompatible pointer type > evas_engine.c:855: warning: passing arg 4 of > `evas_common_draw_context_font_ext_set' from incompatible pointer type > evas_engine.c:855: warning: passing arg 5 of > `evas_common_draw_context_font_ext_set' from incompatible pointer type > evas_engine.c: In function `module_open': > evas_engine.c:926: warning: assignment from incompatible pointer type same as above - i suspect this is xrender or gl engines - but i also know that this is harmless and can be fixed with a cast - but it just makes the code longer to shut the compiler up. > and: > evas_engine_font.c: In function `_xre_font_surface_draw': > evas_engine_font.c:178: warning: comparison is always false due to > limited range of data type > evas_engine_font.c:178: warning: comparison is always false due to > limited range of data type harmless. we are using a common macro for rect clip/reduction and some values are unsigned not signed, thus the comparison will be useless. the macro handles signed cases too and is generic, so this can be ignored. > and:a couple of warnings about "HAVE_STDLIB_H" being redefined in the > jpg folder. jpg - libjpeg. not us. :) > In ecore: > It seems ecore does not respect :--disable-ecore-evas-sdl > --disable-ecore-sdl and still links to SDL if it exist, can someone > please test this under linux. yes. a problem on our part - it links to libsdl - but doesnt use it. fixed. > Unsafe functions strcpy,sprintf and strcat instead of strlcpy,snprintf > and strlcat > > In edje: > edje_data.c: In function `_edje_edd_setup': > edje_data.c:70: warning: assignment from incompatible pointer type > edje_data.c:72: warning: assignment from incompatible pointer type > edje_data.c:73: warning: assignment from incompatible pointer type > edje_data.c:74: warning: assignment from incompatible pointer type > edje_data.c:75: warning: assignment from incompatible pointer type > edje_data.c:76: warning: assignment from incompatible pointer type > edje_data.c:77: warning: assignment from incompatible pointer type > edje_data.c:78: warning: assignment from incompatible pointer type all harmless. it's just bitching about const char * vs char * for generic string return callback prototypes etc. > Unsafe functions:strcpy and strcat. as per eet. i'll make it shorter. all of them are ok. check the code. even comments to the effect that the buffers are checked in the caller and are safe - except one use case - alloca needed to alloc 1 more byte. there more funk there than just looking at strcpy use though. > In embryo: > unsafe functions: strcpy,sprintf and strcat. if it's embryo_cc - forget that. that is a pile of most ugly code inherited from small/pawn and fixed to be 64bit clean and endianess-clean. i do not desire to run through all its snprintf, strcpy etc. uses as it's only used as a stand-alone utility to compile embryo script by edje_cc into .edj files. if someone has nothing to do with their weekends and evenings - they can take this on as a project :) as for the lib dir (libembryo) which is much more important as it is used runtime and loads and runs embryo/small/pawn bytecode and is meant to be entirely safe and sandboxed - no embryo script should ever bring down any process using it. the strcpy/cat's are safe if you check the code itself and the surrounding code that allocs or provides sufficiently large buffers. i fixed a simple path strcpy/cat thing in embryo_cc code in bin - but really. that code is a jungle. > In E: > --- configure.in.orig Wed Nov 7 11:45:47 2007 > +++ configure.in Wed Dec 5 21:29:17 2007 > @@ -70,7 +70,7 @@ AC_CHECK_HEADERS(security/pam_appl.h, [ > AC_SUBST(VALGRIND_CFLAGS) > AC_SUBST(VALGRIND_LIBS) > > -MODULE_ARCH="$host_os-$host_cpu" > +MODULE_ARCH="" > AC_SUBST(MODULE_ARCH) > AC_DEFINE_UNQUOTED(MODULE_ARCH, "$MODULE_ARCH", "Module architecture") again - breaks multi-arch support for e like for evas. > @@ -148,6 +148,9 @@ AC_SUBST(ALL_LINGUAS) > > AM_GNU_GETTEXT([external]) > AM_GNU_GETTEXT_VERSION([0.12.1]) > +LIBICONV="$LTLIBICONV" > +LIBINTL="$LTLIBINTL" > +INTLLIBS="$LTLIBINTL" > if test "x$LIBINTL" = "x"; then > LIBINTL="$INTLLIBS" > fi i am dubious about this - why? > --- src/bin/Makefile.am.orig Fri Nov 9 16:20:53 2007 > +++ src/bin/Makefile.am Fri Nov 9 16:21:15 2007 > @@ -345,7 +345,7 @@ e_xinerama.c > > enlightenment_init_LDFLAGS = @e_libs@ > > -setuid_root_mode = a=rx,u+xs > +setuid_root_mode = a=rx,u+x > install-exec-hook: > @chmod $(setuid_root_mode) > $(DESTDIR)$(bindir)/enlightenment_sys$(EXEEXT) || true you just broke sysactions. e can't shut down, reboot, suspend or hubernate now. it has to be setuid root to be able to run shutdown, reboot etc. as root. there is a sysactions.cfg file- text, that e installs that allows packagers to define access policies and what users, groups etc. can do what and what commands are mapped to which sysaction. you probably should solve this with packaging and install an appropriate sysactions.cfg into openbsd that still allows your average desktop used to do these things as it is a necessary function of using your pc (in almost all if not all cases - if you have access to the power button - you should have access to these). the default sysactions.cfg is set up to "just work" as best as possible and works "as-is" on ubuntu - i guarantee. distributions may/should customise it if needed. > ##This is only to disaable the moduyles because they are not yet > supported, work on this has been stalled. > --- src/bin/e_config.c.orig Fri Dec 20 17:07:06 2007 > +++ src/bin/e_config.c Fri Dec 20 17:07:48 2007 > @@ -766,9 +766,9 @@ e_config_init(void) > CFG_MODULE("ibox", 1, 0); > CFG_MODULE("dropshadow", 1, 0); > CFG_MODULE("clock", 1, 0); > - CFG_MODULE("battery", 1, 0); > - CFG_MODULE("cpufreq", 1, 0); > - CFG_MODULE("temperature", 1, 0); > + CFG_MODULE("battery", 0, 0); > + CFG_MODULE("cpufreq", 0, 0); > + CFG_MODULE("temperature", 0, 0); > CFG_MODULE("pager", 1, 0); > CFG_MODULE("exebuf", 1, 1); > CFG_MODULE("winlist", 1, 1); not useful. these are needed for linux - and eventually the wizard will enable or disable modules based on initial system probes the first time you run E. i.e - check if you have any battery (look for apm, acpi etc.) and if not - dont load battery module. check for temp sensors - if you don't have any e knows about - don't load temp module etc. > --- src/bin/e_fm.c.orig Wed Nov 7 11:45:43 2007 > +++ src/bin/e_fm.c Fri Dec 7 15:44:09 2007 > @@ -104,7 +104,7 @@ struct _E_Fm2_Smart_Data > E_Drop_Handler *drop_handler; > E_Fm2_Icon *drop_icon; > E_Fm2_Mount *mount; > - char drop_after; > + signed char drop_after; > unsigned char drop_show : 1; > unsigned char drop_in_show : 1; > unsigned char drop_all : 1; why? char is normally signed. are you using a strange architecture? i can see how this would break if char is not signed on a particular arch. > @@ -3214,7 +3214,7 @@ _e_fm2_uri_parse(const char *val) > p = val + 7; > if (*p != '/') > { > - for (i = 0; *p != '/' && *p != '\0' && i < > _POSIX_HOST_NAME_MAX; p++, i++) > + for (i = 0; *p != '/' && *p != '\0' && i < MAXHOSTNAMELEN; p++, i++) > hostname[i] = *p; > } > hostname[i] = '\0'; ok - we are swapping one define for another. we are using the POSIX one. if bsd is not supplying posix defines... i'd suggest providing an autoconf check that if _POSIX_HOST_NAME_MAX is not defined in system headers, to define it in config.h > @@ -3247,9 +3247,9 @@ _e_fm2_uri_path_list_get(Evas_List *uri_list) > { > E_Fm2_Uri *uri; > Evas_List *l, *path_list = NULL; > - char current_hostname[_POSIX_HOST_NAME_MAX]; > + char current_hostname[MAXHOSTNAMELEN]; > > - if (gethostname(current_hostname, _POSIX_HOST_NAME_MAX) == -1) > + if (gethostname(current_hostname, MAXHOSTNAMELEN) == -1) > current_hostname[0] = '\0'; > > for (l = uri_list; l; l = l->next) > same here. > $OpenBSD: patch-src_bin_e_main_c,v 1.1 2007/12/04 17:43:15 bernd Exp $ > > The OpenBSD tar(1) doesn't have a -k option to preserve existing files. > Use pax(1) with its -k option. and linux doesn't have pax. this breaks linux badly. also note - these "untar" bits of code are just temporary during development. the wizard will take over the job of populating user config files in the end. so i'm not concerned about this. > --- src/bin/e_main.c.orig Mon Dec 3 13:59:16 2007 > +++ src/bin/e_main.c Mon Dec 3 13:59:48 2007 > @@ -1088,13 +1088,13 @@ _e_main_dirs_init(void) > { > snprintf(buf, sizeof(buf), > "gzip -d -c < %s/data/other/desktop_files.tar.gz | " > - "(cd %s/applications/ ; tar -xkf -)", > + "(cd %s/applications/ ; /bin/pax -rk)", > e_prefix_data_get(), > efreet_data_home_get()); > system(buf); > snprintf(buf, sizeof(buf), > "gzip -d -c < %s/data/other/desktop_order.tar.gz | " > - "(cd %s/.e/e/ ; tar -xkf -)", > + "(cd %s/.e/e/ ; /bin/pax -rk)", > e_prefix_data_get(), > homedir); > system(buf); > @@ -1105,7 +1105,7 @@ _e_main_dirs_init(void) > { > snprintf(buf, sizeof(buf), > "gzip -d -c < %s/data/other/efm_favorites.tar.gz | " > - "(cd %s/.e/e/ ; tar -xkf -)", > + "(cd %s/.e/e/ ; /bin/pax -rk)", > e_prefix_data_get(), > homedir); > system(buf); > > > --- src/modules/cpufreq/Makefile.am.orig Fri Nov 9 15:16:55 2007 > +++ src/modules/cpufreq/Makefile.am Fri Nov 9 15:17:26 2007 > @@ -28,7 +28,7 @@ freqsetdir = $(pkgdir) > freqset_DATA = \ > freqset$(EXEEXT) > > -setuid_root_mode = a=rx,u+xs > +setuid_root_mode = a=rx,u+x > install-data-hook: > @chmod $(setuid_root_mode) > $(DESTDIR)$(freqsetdir)/freqset$(EXEEXT) || true breaks cpufreq! > Warnings: > Alot of unsafe functions. details please. if it's anything like your list of unsafe str* sprintf* funcs above - 90%+ are likely safe - if not all. just because a func is used does not make it unsafe. it is dependent on preconditions (eg the buffer is large enough for its work). i normally make fastidiously sure this *IS* the case. so please list in detail - and actually examine the code for the use cases (so i don't have to keep doing it! :)). > e17 was tested under the following arch: > i386, amd64,zaurus,macppc. > > Also many thanks to all the people that have helped > (patches/testing/etc), too numerous to mention here but you know who > you are. cool. thanks for this - but, you may want to really take note of why something is there in the first place - most of these patches/notes just don't help. they break e or evas entirely, or break support, or break other os/architectures, or are simply reading out compiler warnings without actually checking whats behind them (trust me - we can read compiler warnings too :) vast likelihood is they are still there because they have been looked at and then ignored henceforth as they are safe). also remember that a use of strcpy/cat/sprints is by itself not unsafe. its preconditions need to be met. chances are mostly that we did that - thus i would say the onus is on those reporting to check the instance of that use in detail - then if it violates preconditions, report. :) -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) [EMAIL PROTECTED] ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
