Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14980 --- Ship it! Ship It! - Mark Michelson On March 28, 2015, 3:08 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 28, 2015, 3:08 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 8:23 a.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, lines 960-961 https://reviewboard.asterisk.org/r/4529/diff/2/?file=72981#file72981line960 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; Diederik de Groot wrote: Thanks for the pointer ! No need to count the string length twice, just a null pointer check should suffice. Also need to set the escaped_len to 1 if appdata==NULL, so that it will be able to contain '\0'. rmudgett wrote: ast_strlen_zero() is a boolean test. It does not actually do a strlen. It is just: (!str || (*str == '\0')). Diederik de Groot wrote: Ahh ok i didn't know that. It does have the same effect to my version though (in this particular case: even if appdata[0]='\0' it would still result in escaped_len=1). If you guys prefer the ast_strlen_zero test instead, i will update it though. Since the Asterisk project is over 15 years old and has had a _lot_ of contributors, we care a lot about making sure that the code uses the same techniques and approaches throughout the code base. Otherwise, anyone who wants to contribute a patch would have to deal with numerous different conflicting styles. Granted, we still deal with this sometimes, as we didn't start caring that much about this until it started to bite us, but we definitely care now! - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- On March 27, 2015, 11:32 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 11:32 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 2:23 p.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, lines 960-961 https://reviewboard.asterisk.org/r/4529/diff/2/?file=72981#file72981line960 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; Diederik de Groot wrote: Thanks for the pointer ! No need to count the string length twice, just a null pointer check should suffice. Also need to set the escaped_len to 1 if appdata==NULL, so that it will be able to contain '\0'. rmudgett wrote: ast_strlen_zero() is a boolean test. It does not actually do a strlen. It is just: (!str || (*str == '\0')). Diederik de Groot wrote: Ahh ok i didn't know that. It does have the same effect to my version though (in this particular case: even if appdata[0]='\0' it would still result in escaped_len=1). If you guys prefer the ast_strlen_zero test instead, i will update it though. Matt Jordan wrote: Since the Asterisk project is over 15 years old and has had a _lot_ of contributors, we care a lot about making sure that the code uses the same techniques and approaches throughout the code base. Otherwise, anyone who wants to contribute a patch would have to deal with numerous different conflicting styles. Granted, we still deal with this sometimes, as we didn't start caring that much about this until it started to bite us, but we definitely care now! Meaning i should change this to use ast_strlen_zero in this case... right ? - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- On March 27, 2015, 5:32 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 5:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 28, 2015, 4:08 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs (updated) - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 2:11 a.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, line 977 https://reviewboard.asterisk.org/r/4529/diff/1/?file=72935#file72935line977 So, dup'ing things on the stack here in a nested loop is actually quite dangerous. Unlike a local variable with scope within the loop block, a variable that is allocated with one of the alloca methods (which strdupa should fall into) does not have its memory reclaimed when it loses block scope. It is only reclaimed when the stack frame returns. As such, this change could overrun the stack. Since escaped is a char *, I'd go ahead and strdup it here, and free it before the loop continues. Diederik de Groot wrote: Ooh damn, you are right, missed that one. Maybe someone can come up with another way where this tmp_escaped is not necessary at all. There was a less complex way afterall - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14874 --- On March 27, 2015, 11:22 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 11:22 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 11:22 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs (updated) - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 2:11 a.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, line 977 https://reviewboard.asterisk.org/r/4529/diff/1/?file=72935#file72935line977 So, dup'ing things on the stack here in a nested loop is actually quite dangerous. Unlike a local variable with scope within the loop block, a variable that is allocated with one of the alloca methods (which strdupa should fall into) does not have its memory reclaimed when it loses block scope. It is only reclaimed when the stack frame returns. As such, this change could overrun the stack. Since escaped is a char *, I'd go ahead and strdup it here, and free it before the loop continues. Ooh damn, you are right, missed that one. Maybe someone can come up with another way where this tmp_escaped is not necessary at all. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14874 --- On March 26, 2015, 7:03 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 26, 2015, 7:03 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- /branches/13/pbx/pbx_config.c https://reviewboard.asterisk.org/r/4529/#comment25519 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; - Matt Jordan On March 27, 2015, 5:22 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 5:22 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 2:23 p.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, lines 960-961 https://reviewboard.asterisk.org/r/4529/diff/2/?file=72981#file72981line960 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; Diederik de Groot wrote: Thanks for the pointer ! No need to count the string length twice, just a null pointer check should suffice. Also need to set the escaped_len to 1 if appdata==NULL, so that it will be able to contain '\0'. rmudgett wrote: ast_strlen_zero() is a boolean test. It does not actually do a strlen. It is just: (!str || (*str == '\0')). Ahh ok i didn't know that. It does have the same effect to my version though (in this particular case: even if appdata[0]='\0' it would still result in escaped_len=1). If you guys prefer the ast_strlen_zero test instead, i will update it though. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- On March 27, 2015, 5:32 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 5:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 8:23 a.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, lines 960-961 https://reviewboard.asterisk.org/r/4529/diff/2/?file=72981#file72981line960 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; Diederik de Groot wrote: Thanks for the pointer ! No need to count the string length twice, just a null pointer check should suffice. Also need to set the escaped_len to 1 if appdata==NULL, so that it will be able to contain '\0'. ast_strlen_zero() is a boolean test. It does not actually do a strlen. It is just: (!str || (*str == '\0')). - rmudgett --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- On March 27, 2015, 11:32 a.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 11:32 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
On March 27, 2015, 2:23 p.m., Matt Jordan wrote: /branches/13/pbx/pbx_config.c, lines 960-961 https://reviewboard.asterisk.org/r/4529/diff/2/?file=72981#file72981line960 Well, this won't quite work, as appdata can be NULL. strlen isn't NULL safe. As such, you'll need to do something like: int escaped_len = (!ast_strlen_zero(appdata) ? 2 * strlen(appdata) : 0) + 1; Thanks for the pointer ! No need to count the string length twice, just a null pointer check should suffice. Also need to set the escaped_len to 1 if appdata==NULL, so that it will be able to contain '\0'. - Diederik --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14894 --- On March 27, 2015, 5:32 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 5:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 27, 2015, 5:32 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs (updated) - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4529: clang compiler warning: -Wno-sometimes-uninitialized
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/#review14874 --- /branches/13/pbx/pbx_config.c https://reviewboard.asterisk.org/r/4529/#comment25502 So, dup'ing things on the stack here in a nested loop is actually quite dangerous. Unlike a local variable with scope within the loop block, a variable that is allocated with one of the alloca methods (which strdupa should fall into) does not have its memory reclaimed when it loses block scope. It is only reclaimed when the stack frame returns. As such, this change could overrun the stack. Since escaped is a char *, I'd go ahead and strdup it here, and free it before the loop continues. - Matt Jordan On March 26, 2015, 1:03 p.m., Diederik de Groot wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4529/ --- (Updated March 26, 2015, 1:03 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24917 https://issues.asterisk.org/jira/browse/ASTERISK-24917 Repository: Asterisk Description --- clang's static analyzer will throw quite a number warnings / errors during compilation, some of which can be very helpfull in finding corner-case bugs\nclang compiler warning:-Wno-sometimes-uninitialized Diffs - /branches/13/pbx/pbx_config.c 433444 Diff: https://reviewboard.asterisk.org/r/4529/diff/ Testing --- Thanks, Diederik de Groot -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
