| 01:00.11 | *** join/#asterisk-dev n8ideas (~josh@50.141.117.45) |
| 01:02.08 | *** join/#asterisk-dev dms (~Adium@c-73-131-162-128.hsd1.sc.comcast.net) |
| 03:05.28 | *** join/#asterisk-dev dms (~Adium@c-73-131-162-128.hsd1.sc.comcast.net) |
| 04:52.43 | *** join/#asterisk-dev leedm777 (~leedm777@50-81-28-152.client.mchsi.com) |
| 05:09.58 | *** join/#asterisk-dev babak (uid19622@gateway/web/irccloud.com/x-egugwtfhdbkjmnzk) |
| 07:56.14 | *** join/#asterisk-dev oej (~oej@h87-96-134-129.dynamic.se.alltele.net) |
| 07:56.14 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 08:13.47 | *** join/#asterisk-dev tzafrir (~tzafrir@local.xorcom.com) |
| 08:13.47 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |
| 12:51.34 | *** join/#asterisk-dev n8ideas (~josh@l136.denver.dsl.forethought.net) |
| 13:58.04 | *** join/#asterisk-dev [TK]D-Fender (~joe@64.235.216.2) |
| 15:11.11 | *** join/#asterisk-dev jameswf (uid27319@gateway/web/irccloud.com/x-nucxbozpexzghenw) |
| 17:00.18 | coreyfarrell | nbjoerg: only the author of a patch is allowed to submit to the Asterisk project.. assuming you wrote that patch you would need to submit to JIRA or gerrit before anyone else can consider working on it. |
| 17:05.29 | nbjoerg | I am, it is more a question of whether you care enough to justify spending more time on it |
| 17:06.23 | *** join/#asterisk-dev n8ideas (~josh@216.241.47.214) |
| 17:07.35 | coreyfarrell | to be honest I'm not sure. can you explain to be the benefits? I mean why do I want this. also what do you mean by strict aliasing violation. |
| 17:09.52 | nbjoerg | it removes the need for nested functions with GCC, which should make binaries a bit smaller and much more importantly, removes the need for executable stacks |
| 17:10.15 | nbjoerg | for clang, it removes the need for the blocks runtime, which not every clang installation has |
| 17:11.38 | nbjoerg | the strict aliasing violation happens because the dtor gets a foo** argument and sadly C has no wat to get that dereferencced to void * |
| 17:12.41 | nbjoerg | it could be avvoided by changing all RAII_VAR instances to provide the base type as additional tag, but that's messy and makes the patch *much* larger |
| 17:13.29 | nbjoerg | [essentially, provide something that can be used with the preprocessor to form a combined dtor name, which the type normally ddoesn't allow] |
| 17:13.29 | coreyfarrell | 'removes the need for executable stacks' - is this a security / binary fortification thing? |
| 17:13.38 | nbjoerg | yes |
| 17:13.54 | nbjoerg | nested functions in C work by creating a trampoline on the stack |
| 17:15.13 | nbjoerg | so the patch would help portability and reduce attack surface for GCC users |
| 17:15.29 | coreyfarrell | ahh.. so I've looked at compiling asterisk with stack overflow protection (don't remember the option), it failed due to uses of alloca type allocations.. is it possible to get the benefit of no trampoline without full stack overflow protection? |
| 17:15.56 | nbjoerg | the two are not related |
| 17:16.36 | nbjoerg | stackprotector not working with alloca is a GCC bug, but completely different area of code :) |
| 17:19.45 | coreyfarrell | I would suggest creating a JIRA ticket and uploading the patch as is... personally I don't care about the portability issue that would be fixed by this, but removal of trampolines would be nice if it can be done in a reasonable way. |
| 17:22.45 | coreyfarrell | not sure something like this would be considered for already released branches, but I think we could look into it for master |
| 17:23.15 | WIMPy | 'd file that as a security fix. |
| 17:23.24 | WIMPy | But it looks like I really need to learn more gcc. |
| 17:25.12 | gtjoseph | nbjoerg: I might have some time to explore/help with this. How did you determine where to use the DTOR macros? |
| 17:26.34 | nbjoerg | gtjoseph: basically, if it is a common type next to where it is declared |
| 17:26.46 | nbjoerg | gtjoseph: with the associated destruction function |
| 17:26.55 | nbjoerg | and whatever is left in the corresponding sources |
| 17:27.36 | gtjoseph | gotcha |
| 17:29.11 | nbjoerg | there is one small part that was on gerrit before (573) |
| 17:29.22 | nbjoerg | should now be more obvious why the change was desirable :) |
| 17:31.02 | gtjoseph | rmudgett will definitely have more to say. :) |
| 17:31.49 | nbjoerg | sure |
| 17:33.12 | gtjoseph | which version of gcc and clang did you test with? |
| 17:36.23 | nbjoerg | mostly trunk for clang and gcc 4.8 |
| 17:36.37 | gtjoseph | ah. getting errors in gcc 5.1.1 |
| 17:36.44 | nbjoerg | that said, full testing is still a bit limited |
| 17:36.50 | nbjoerg | what does it complain about? |
| 17:37.14 | nbjoerg | (might be some code path I simply didn't hit) |
| 17:37.42 | *** join/#asterisk-dev dms (~Adium@c-73-131-162-128.hsd1.sc.comcast.net) |
| 17:37.55 | gtjoseph | http://pastebin.com/2W1wmENx |
| 17:38.39 | nbjoerg | res_pjsip needs an external dependency now, doesn't it? |
| 17:38.53 | gtjoseph | pjproject |
| 17:39.44 | nbjoerg | right, thought so |
| 17:39.46 | coreyfarrell | res_pjsip always needed external pjproject |
| 17:41.43 | nbjoerg | hm. need to check the bridge_basic.c failure |
| 17:48.17 | coreyfarrell | nbjoerg: changing how the RAII_VAR macro works is very disruptive. might be easier to move forward if the way RAII_VAR worked was unchanged, and a different macro name were used for the new way to do it. |
| 17:51.14 | coreyfarrell | if you change how RAII_VAR works then this is all or nothing. but if you create a new macro name uses of RAII could be changed one type at a time, or one source at a time.. |
| 17:53.48 | nbjoerg | yes, but on the other hand it makes it much easier to have a switch in one place to select either old or new |
| 17:55.32 | gtjoseph | being able to switch easily will make testing easier |
| 18:00.55 | coreyfarrell | RAII_VAR is used in about 240 sources... if RAII_VAR changes wouldn't they all have to be modified in a single patch? |
| 18:03.00 | gtjoseph | if the prototype dosesn't change? |
| 18:03.34 | nbjoerg | I think it wouold be more distracting |
| 18:05.59 | coreyfarrell | oh sorry so you would only need to add an RAII_DTOR for every type that uses RAII_VAR, I see that now |
| 18:10.04 | nbjoerg | yes |
| 18:11.36 | nbjoerg | so the biggest prep right now is getting the scoped_lock handling for rwlist cleaneed up, that's a bit nasty |
| 18:25.54 | coreyfarrell | nbjoerg: do you know is there a gcc flag to cause error's if trampolines are attempted? |
| 18:28.35 | nbjoerg | don't think so |
| 18:29.29 | nbjoerg | IIRC there isn't even an isolated options for warning about nested functions |
| 18:41.54 | nbjoerg | gtjoseph: any special flags |
| 18:42.08 | gtjoseph | for what? |
| 18:42.22 | nbjoerg | for the above compile error? |
| 18:42.38 | gtjoseph | hold on... i'll give you my ./configure |
| 18:43.15 | gtjoseph | actually, it's just --enable-dev-mode --with-ssl |
| 18:43.26 | nbjoerg | let me try that |
| 18:43.46 | gtjoseph | start from a make distclean |
| 18:43.51 | nbjoerg | yeah |
| 18:44.07 | gtjoseph | nothing special in menuselect |
| 18:46.26 | nbjoerg | res_pjsip_session.c:678:2: error: implicit declaration of function âpj_timer_entry_runningâ [-Werror=implicit-function-declaration] |
| 18:46.31 | nbjoerg | that seems unrelated |
| 18:46.48 | gtjoseph | it is...where did you get pjproject? |
| 18:46.58 | gtjoseph | and what version |
| 18:46.59 | nbjoerg | whatever is in ubuntu |
| 18:47.07 | nbjoerg | but the problem is unt that |
| 18:47.26 | nbjoerg | or it is |
| 18:58.48 | nbjoerg | ok, I can reproduce it with dev-mode, still a bit puzzled why it doesn't happen in normal mode |
| 18:58.55 | coreyfarrell | nbjoerg: see https://wiki.asterisk.org/wiki/display/AST/Building+and+Installing+pjproject |
| 18:59.31 | coreyfarrell | make sure you uninstall the ubuntu package first.. |
| 19:00.21 | gtjoseph | or move to Fedora and use jsmith's excellent packages. :) |
| 19:02.04 | nbjoerg | updated diff uploaded |
| 19:02.18 | gtjoseph | same url? |
| 19:02.18 | nbjoerg | still need to contemplate why it works in non-dev-mode without the cast |
| 19:04.03 | coreyfarrell | dev-mode enables additional errors/warnings |
| 19:04.18 | gtjoseph | and sets warnings as errors |
| 19:05.00 | nbjoerg | I don't get a warning at all |
| 19:05.18 | nbjoerg | I was more wondering whether there is a mutex debugging thing enabled that I missed |
| 19:06.29 | gtjoseph | is your new patch at the same url? |
| 19:07.50 | gtjoseph | nbjoerg: tons of "error: cleanup argument not a function" |
| 19:09.55 | nbjoerg | sec, let me double check that git didn't mess things up |
| 19:10.39 | nbjoerg | http://www.netbsd.org/~joerg/asterisk-raii.diff |
| 19:10.50 | gtjoseph | since you have a gerrit account you should create the review with a title of "RFC: ...". It'll be easier. |
| 19:11.12 | nbjoerg | sure |
| 19:13.35 | gtjoseph | nbjoerg: Much better only getting res_config_pgsql.c:256:2: error: cleanup argument not a function |
| 19:13.46 | gtjoseph | plus another 7 in that file |
| 19:14.05 | nbjoerg | ah, right |
| 19:14.15 | nbjoerg | don't have the dependencies installed for that |
| 19:14.44 | gtjoseph | what do i need to do? |
| 19:16.41 | nbjoerg | RAII_DTOR(PGresult*, PQclear); |
| 19:16.52 | gtjoseph | yep, got it |
| 19:17.27 | gtjoseph | successful make |
| 19:18.47 | gtjoseph | well, it's running... |
| 19:19.09 | gtjoseph | I'll run ref_debuf later this afternoon |
| 19:19.41 | gtjoseph | in the mean time, think of how to test the change other than run the existing tests |
| 19:21.36 | coreyfarrell | I think running one test in valgrind (compare leaks to unpatched) will cover it. the testsuite without memory debugging is probably the best way to verify the locking still works. |
| 19:22.53 | nbjoerg | the scoped lock changes are the one part that should be separated out and carefully double checked for typos |
| 19:23.25 | gtjoseph | i just found a bunch more places RAII_DTOR is needed. |
| 19:23.33 | coreyfarrell | only other thing I can think of is somehow verifying that this doesn't change the order of cleanups. |
| 19:24.34 | gtjoseph | nbjoerg: turn on the TEST_FRAMEWORK and compile all the tests. |
| 19:26.54 | nbjoerg | ack |
| 19:27.21 | gtjoseph | i'm heading out for a while but if you put the patch up on gerrit it'll be easier for us to make changes |
| 19:27.29 | gtjoseph | and test |
| 19:27.47 | nbjoerg | about to go to sleep, will do that tomorrow |
| 19:28.32 | gtjoseph | No worries! |
| 20:13.58 | *** join/#asterisk-dev tzafrir (~tzafrir@bzq-179-40-172.cust.bezeqint.net) |
| 20:13.58 | *** mode/#asterisk-dev [+o tzafrir] by ChanServ |
| 20:31.37 | *** join/#asterisk-dev n8ideas (~josh@l136.denver.dsl.forethought.net) |
| 20:40.24 | *** join/#asterisk-dev oej (~oej@h87-96-134-129.dynamic.se.alltele.net) |
| 20:40.24 | *** mode/#asterisk-dev [+o oej] by ChanServ |
| 21:31.40 | *** join/#asterisk-dev n8ideas (~josh@l136.denver.dsl.forethought.net) |
| 22:31.39 | gtjoseph | coreyfarrell: still around? |
| 22:48.09 | coreyfarrell | <PROTECTED> |
| 22:48.27 | gtjoseph | have you looked at nbjoerg's implementation? |
| 22:48.38 | gtjoseph | riaa.h |
| 22:48.43 | gtjoseph | raii.h |
| 22:49.33 | coreyfarrell | on the URL but let me download the patch so I can get a better view |
| 22:49.51 | gtjoseph | a coupe of things concern me with the implementation |
| 22:50.10 | gtjoseph | the patch should apply cleanly on master |
| 22:51.14 | coreyfarrell | yup I just got a working copy going and applied it |
| 22:51.47 | coreyfarrell | anyplace you want me to start? |
| 22:52.01 | gtjoseph | just look at raii.h |
| 22:52.34 | gtjoseph | the destructors that can hangle multiple types are defined using the RAII_DENERIC_DTOR |
| 22:52.44 | gtjoseph | i.e ao2_cleanup |
| 22:53.04 | gtjoseph | but that does a memcpy every time the destructor is invoked |
| 22:54.34 | coreyfarrell | I think memcpy is being used to break the strict aliasing rules |
| 22:55.05 | gtjoseph | yep and it's only the size of a pointer but... |
| 22:55.15 | gtjoseph | ao2_cleanup and ast_free get used a lot |
| 22:56.16 | coreyfarrell | yeah.. I'm not sure I agree with having the generic dtor's |
| 22:56.46 | gtjoseph | the flip side is that the way it's implemented, you can't use the same destructor for 2 different types in the same source file |
| 22:57.04 | gtjoseph | if you don't use the generic that is |
| 22:57.06 | coreyfarrell | also "_raii_dtor_ ## dtor" seems like the wrong name to use for the function. I think "_raii_dtor_ ## vartype" would be more appropriate |
| 22:57.23 | gtjoseph | either way, we've got collisions |
| 22:57.24 | coreyfarrell | this would make generic dtor's impossible though |
| 22:58.13 | coreyfarrell | I don't think we have any vartype with multiple options for dtor functions though |
| 22:58.56 | gtjoseph | to use vartype wouldn't you have to convert it to an identifier? |
| 22:59.31 | coreyfarrell | oh right vartype has 'struct whatever *', sorry I was thinking it was 'whatever' |
| 22:59.49 | gtjoseph | yeah |
| 22:59.52 | gtjoseph | :) |
| 23:01.10 | gtjoseph | Also, the tests are full of custom destructors that's need to be RAII_DTORed. I gave up and will let nbjoerg do it. :) |
| 23:03.20 | coreyfarrell | so I was thinking of: |
| 23:03.31 | coreyfarrell | #define RAII_STRUCT_DTOR(vartype, dtor) static void __attribute__((__unused__)) _raii_dtor_ ## vartype(struct vartype **p) { dtor(*p); } |
| 23:03.52 | coreyfarrell | #define RAII_STRUCT(vartype, varname, initval) struct vartype *varname __attribute__((cleanup(_raii_dtor_ ## vartype))) = (initval) |
| 23:04.34 | coreyfarrell | but this goes back to what I was saying about changing one type at a time / creating a new way of doing RAII |
| 23:04.41 | gtjoseph | we have structs that are typedef'd as well. |
| 23:04.55 | gtjoseph | and non struct things |
| 23:04.58 | gtjoseph | like char * |
| 23:06.00 | gtjoseph | If this proved useful and the memcpy wasn't an issue I'd almost rather create a macro like AST_DTOR_DEFINE that uses the generic form |
| 23:07.04 | gtjoseph | I.E. AST_DTOR_DEFINE(my_cleanup, struct mystruct * m) {... |
| 23:07.58 | gtjoseph | that places the wrapper with the memcpy after my _cleanup |
| 23:09.09 | coreyfarrell | not sure I follow |
| 23:10.06 | gtjoseph | i'm working on an example... |
| 23:15.31 | coreyfarrell | #define RAII_GENERIC_DTOR(dtor) static void __attribute__((__unused__)) _raii_dtor_ ## dtor(void *v) { void **v2 = (void**)v; dtor(*v2); } |
| 23:15.52 | gtjoseph | yeah, i was thinking about a cast |
| 23:15.56 | coreyfarrell | gtjoseph: ^^ this works on my compiler, but I think it's technically a violation of some C spec |
| 23:17.17 | gtjoseph | http://pastebin.com/4cPdxVRV |
| 23:18.40 | *** join/#asterisk-dev dms (~Adium@c-73-131-162-128.hsd1.sc.comcast.net) |
| 23:21.35 | gtjoseph | coreyfarrell: https://pastebin.com/4MwgfZHj |
| 23:23.48 | coreyfarrell | i'm a little confused it looks like that has some errors |
| 23:24.03 | gtjoseph | i wrote it freehand. :) |
| 23:24.13 | gtjoseph | just to give you an idea |
| 23:27.42 | gtjoseph | basically AST_DTOR_DEFINE does a RAII_GENERIC_DTOR for you and sets a RAII_NEW define so that later calls to RAII_VAR can use the old and new version |
| 23:29.04 | coreyfarrell | given the parameters you setup for AST_DTOR_DEFINE I think it would do non-generic |
| 23:30.00 | gtjoseph | no because the actual __raii_dtor_ definition doesn't use vartype |
| 23:33.05 | gtjoseph | check the pb again |
| 23:33.20 | gtjoseph | this one ... https://pastebin.com/4MwgfZHj |
| 23:33.52 | gtjoseph | still just typing freehand |
| 23:34.36 | coreyfarrell | yeah, unfortunately #define and such can't be used inside a macro :( |
| 23:35.28 | gtjoseph | oh, yeah. |
| 23:42.41 | coreyfarrell | gtjoseph: http://pastebin.com/HuaywKzL |
| 23:44.10 | gtjoseph | looks like more work to implement though. |
| 23:44.17 | gtjoseph | example? |
| 23:44.53 | coreyfarrell | oops.. http://pastebin.com/VUNu51us |
| 23:46.57 | coreyfarrell | gtjoseph: http://pastebin.com/wCuL08SG |
| 23:47.03 | gtjoseph | does this actually prevent the trampoline though? |
| 23:48.28 | coreyfarrell | no nested functions.. AST_RAII_DECLARE would be at the top of a source for a private type or in a header |
| 23:51.32 | gtjoseph | i wasn't actually following the original issue. :) but the efffect is achieved by the cleanup function being static in the same source file even though the cleanup function call a global function. |
| 23:51.34 | gtjoseph | ? |
| 23:52.21 | coreyfarrell | a trampoline is caused by the nested function (that's my understanding anyhow) |
| 23:52.39 | gtjoseph | time to read up i guess |
| 23:52.43 | coreyfarrell | I'm not 100% sure on that though |
| 23:53.07 | coreyfarrell | it could be that a trampoline is only caused when a nested function uses variables from the parent function.. |
| 23:53.22 | WIMPy | stackoverflow seems to agree. |
| 23:53.33 | nbjoerg | correct, the trampoline comes from using nested functions |
| 23:53.39 | gtjoseph | anyway, i'd combine your AST_RAII_DECLARE with my AST_DTOR_DEFINE so there's no separate step required |
| 23:53.54 | nbjoerg | some versions of gcc might optimise it away, but I haven't seen it do that consistently |
| 23:54.05 | gtjoseph | nbjoerg: your awake? |
| 23:54.10 | WIMPy | And there seems to be a binary that can read and write the stack execution bit somewhere. |
| 23:54.13 | nbjoerg | woke up a bit :) |
| 23:54.26 | coreyfarrell | gtjoseph: yeah see the latest paste it's combined |
| 23:54.31 | nbjoerg | WIMPy: there's a helper function in libgcc for it |
| 23:55.04 | coreyfarrell | I know the following would cause a trampoline? void f1(void) { int i = 1; void f2(void) { printf("%d\n", i); }; f2(); } |
| 23:55.14 | nbjoerg | definitely |
| 23:55.27 | gtjoseph | coreyfarrell: in http://pastebin.com/wCuL08SG ? not seeing it. |
| 23:55.32 | coreyfarrell | but if f2 doesn't use 'i' or any other variables from the parent scope, would that still cause a trampoline? |
| 23:56.24 | coreyfarrell | http://pastebin.com/wCuL08SG only has AST_RAII_DECLARE, no separate AST_RAII_DEFINE |
| 23:57.28 | gtjoseph | yeah, i was suggesting the AST_RAII_DECLARE would also build the function... AST_RAII_DECLARE(...) { my cleanup steps} |
| 23:57.49 | nbjoerg | acutally, it might get inlined |
| 23:58.15 | coreyfarrell | don't think a use of __attribute__((cleanup(dtor))) could inline dtor.. |
| 23:59.04 | gtjoseph | if the dtor was static and in the same source file? only gcc knows for sure. :) |
| 23:59.58 | nbjoerg | that was about one of the earlier examples |