Re: [asterisk-dev] [Code Review] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13932 --- Ship it! Ship It! - opticron On Nov. 14, 2014, 5:03 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 5:03 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427948 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Dec. 9, 2014, 2:46 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 429223 Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427948 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13789 --- Ship it! Ship It! - Corey Farrell On Nov. 14, 2014, 6:03 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 6:03 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427948 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 9:12 a.m.) Review request for Asterisk Developers. Changes --- I also realized I should have gone further with the fix before posting it. I started fixing it another way but prefer Corey's suggestion to my idea. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs (updated) - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13777 --- /branches/13/main/asterisk.c https://reviewboard.asterisk.org/r/4182/#comment24262 Does this actually initialize 256 bytes of '\0', or just initialize the first byte? /branches/13/main/asterisk.c https://reviewboard.asterisk.org/r/4182/#comment24261 Space around '-'. Also why was the return removed? - Corey Farrell On Nov. 14, 2014, 10:12 a.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 10:12 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
On Nov. 14, 2014, 4:08 p.m., Corey Farrell wrote:
/branches/13/main/asterisk.c, line 3203
https://reviewboard.asterisk.org/r/4182/diff/2/?file=68987#file68987line3203
Does this actually initialize 256 bytes of '\0', or just initialize the
first byte?
Initializing a char array with or { 0 } sets the entire array to zero,
whereas the values are undefined otherwise.
On Nov. 14, 2014, 4:08 p.m., Corey Farrell wrote:
/branches/13/main/asterisk.c, lines 3220-3222
https://reviewboard.asterisk.org/r/4182/diff/2/?file=68987#file68987line3220
Space around '-'.
Also why was the return removed?
I have absolutely no idea how that happened other than the vim ghost.
- Scott
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4182/#review13777
---
On Nov. 14, 2014, 5:03 p.m., Scott Griepentrog wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4182/
---
(Updated Nov. 14, 2014, 5:03 p.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
When connecting to the remote console, an identifier string is first provided
that consists of hostname/pid/version. This is parsed by the remote instance
in a buffer allocated to only 80 bytes. It is possible for a combination of
very long hostname and very long asterisk version number to be greater than
80 characters, causing the parsing to fall off the end of the allocated
memory buffer and potentially crash.
This change increases the buffer from 80 to 256 to significantly reduce that
possibility.
Diffs
-
/branches/13/main/asterisk.c 427948
Diff: https://reviewboard.asterisk.org/r/4182/diff/
Testing
---
It stopped crashing on a repeated test I was running where the atoi of the
version # happen to hit the end of the buffer.
Thanks,
Scott Griepentrog
--
_
-- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 14, 2014, 5:03 p.m.) Review request for Asterisk Developers. Changes --- Corrected unintended change. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs (updated) - /branches/13/main/asterisk.c 427948 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13751 --- Could this be solved by: read(ast_consock, buf, sizeof(buf) - 1) ? read() operates on buffers not strings, so it has no concept of null termination. Also I think we need to either memset(buf, 0, sizeof(buf)) before the read, or (better) use the result of read to set the NULL terminator. It looks like we're currently relying on buf to be zero filled before read() when it's actually uninitialized data. I don't have a specific problem with increasing the buffer from 80 to 256, but I think we need to fix the parser so it doesn't crash if given 256 or more bytes. I think this issue applies to 11+. - Corey Farrell On Nov. 13, 2014, 3:31 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 13, 2014, 3:31 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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] 4182: core: avoid rasterisk crash due to long identifier
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/#review13752 --- I typically take issue with patches like this because it's not actually fixing where the crash is occurring. As you said in the review, it reduces the chance of the bug occurring, but it has not mended the defect. - Mark Michelson On Nov. 13, 2014, 8:31 p.m., Scott Griepentrog wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4182/ --- (Updated Nov. 13, 2014, 8:31 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- When connecting to the remote console, an identifier string is first provided that consists of hostname/pid/version. This is parsed by the remote instance in a buffer allocated to only 80 bytes. It is possible for a combination of very long hostname and very long asterisk version number to be greater than 80 characters, causing the parsing to fall off the end of the allocated memory buffer and potentially crash. This change increases the buffer from 80 to 256 to significantly reduce that possibility. Diffs - /branches/13/main/asterisk.c 427813 Diff: https://reviewboard.asterisk.org/r/4182/diff/ Testing --- It stopped crashing on a repeated test I was running where the atoi of the version # happen to hit the end of the buffer. Thanks, Scott Griepentrog -- _ -- 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
