From: Shreyansh J. <shr...@gm...> - 2008-08-27 07:18:22
|
Hi List, Following patch fixes a buffer overrun problem in __dump_line function in log.c file. It also removes a stray 'return' call in log_pdu function which was restricting a PDU dump call to complete log_pdu operation. In dump_line, the array line[] was defined as line[53] whereas sprintf was pushing in 59 characters into it. This was leading to segfault of ietd when debugging was enabled. Also, in log_pdu function, the first check for log_level against passed parameter for log was incorrect and would have forced a return even when the caller has set the logging level to acceptable level. Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> --- Shreyansh --- -------BEGIN------ Index: usr/log.c =================================================================== --- usr/log.c (revision 168) +++ usr/log.c (working copy) @@ -64,7 +64,7 @@ static void __dump_line(int level, unsigned char *buf, int *cp) { - char line[16*3+5], *lp = line; + char line[59], *lp = line; int i, cnt; cnt = *cp; @@ -102,9 +102,8 @@ int char_cnt = 0; unsigned char *buf; int i; - return; - if (log_level <= level) + if (log_level < level) return; buf = (void *)&pdu->bhs; -------END------ |
From: Shreyansh J. <shr...@gm...> - 2008-09-01 07:01:12
|
Hi List, As discussed in this thread, following (updated with comments) patch fixes a buffer overrun problem in __dump_line function in log.c file. It also removes a stray 'return' call in log_pdu function which was restricting a PDU dump call to complete log_pdu operation. In dump_line, the array line[] was defined as line[53] whereas sprintf was pushing in 59 characters into it. This was leading to segfault of ietd when debugging was enabled. Through this patch, the size of line char array would be linked to the size of buffer defined in log_pdu function. The calculation used is (N*3 + ((N/4)+1)*2 + 3), where N is the expected size of buffer used in log_pdu function. Also, in log_pdu function, the first check for log_level against passed parameter for log was incorrect and would have forced a return even when the caller has set the logging level to acceptable level. Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> ---BEGIN--- Index: usr/log.c =================================================================== --- usr/log.c (revision 168) +++ usr/log.c (working copy) @@ -62,15 +62,27 @@ } } +/* Definition for log_pdu buffer */ +#define BUFFER_SIZE 16 +/* Buffer line would be printed in the following format in syslog + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' + * Means, 61 Characters if size of line is 16 chars. For N being the buffer + * size, the fomula would be = (N*3 + ((N/4)+1)*2 + 3), where, the middle term + * is to counter printing " | " after each 4 bytes. Last '3' is to compensate + * for " " printed when (i >= cnt) (and when will that happen??) + */ +#define LINE_SIZE (((BUFFER_SIZE)*3) + \ + ((((int)BUFFER_SIZE/4)+1)*2) + 3) + static void __dump_line(int level, unsigned char *buf, int *cp) { - char line[16*3+5], *lp = line; + char line[LINE_SIZE], *lp = line; int i, cnt; cnt = *cp; if (!cnt) return; - for (i = 0; i < 16; i++) { + for (i = 0; i < BUFFER_SIZE; i++) { if (i < cnt) lp += sprintf(lp, " %02x", buf[i]); else @@ -89,7 +101,7 @@ int cnt = (*cp)++; buf[cnt] = ch; - if (cnt == 15) + if (cnt == (BUFFER_SIZE - 1)) __dump_line(level, buf, cp); } @@ -98,13 +110,12 @@ void log_pdu(int level, struct PDU *pdu) { - unsigned char char_buf[16]; + unsigned char char_buf[BUFFER_SIZE]; int char_cnt = 0; unsigned char *buf; int i; - return; - if (log_level <= level) + if (log_level < level) return; buf = (void *)&pdu->bhs; ---END--- |
From: Arne R. <ag...@po...> - 2008-09-02 18:48:18
|
Hi Shreyansh, > +/* Definition for log_pdu buffer */ > +#define BUFFER_SIZE 16 > +/* Buffer line would be printed in the following format in syslog > + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' > + * Means, 61 Characters if size of line is 16 chars. For N being the buffer > + * size, the fomula would be = (N*3 + ((N/4)+1)*2 + 3), where, the middle term > + * is to counter printing " | " after each 4 bytes. Last '3' is to compensate > + * for " " printed when (i >= cnt) (and when will that happen??) > + */ > +#define LINE_SIZE (((BUFFER_SIZE)*3) + \ > + ((((int)BUFFER_SIZE/4)+1)*2) + 3) > + The formatting of this is rather unusual, #define LINE_SIZE (BUFFER_SIZE * 3 + (BUFFER_SIZE / 4 + 1) * 2 + 3) would be preferable. I'd fix that and apply the patch if it was the only issue, but taking a closer look at your calculation I'm afraid I fail to understand it. IMHO, it should be: for (i = 0; i < BUFFER_SIZE; i++) { if (i < cnt) lp += sprintf(lp, " %02x", buf[i]); else lp += sprintf(lp, " "); => 3 chars / per byte * BUFFER_SIZE, no matter if we're in the if or in the else branch if ((i % 4) == 3) lp += sprintf(lp, " |"); => + 2 chars after each 4th byte of BUFFER_SIZE if (i >= cnt || !isprint(buf[i])) buf[i] = ' '; } log_debug(level, "%s %.16s |", line, buf); => + '\0' termination, of course, so: #define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) , or am I missing something? Thanks, Arne |
From: Shreyansh J. <shr...@gm...> - 2008-09-03 05:37:08
|
Hi Arne and List, Thanks for your valuable time and comments. Please find my reply in line. On Wed, Sep 3, 2008 at 12:18 AM, Arne Redlich <ag...@po...> wrote: > Hi Shreyansh, > >> +/* Definition for log_pdu buffer */ >> +#define BUFFER_SIZE 16 >> +/* Buffer line would be printed in the following format in syslog >> + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' >> + * Means, 61 Characters if size of line is 16 chars. For N being the buffer >> + * size, the fomula would be = (N*3 + ((N/4)+1)*2 + 3), where, the middle term >> + * is to counter printing " | " after each 4 bytes. Last '3' is to compensate >> + * for " " printed when (i >= cnt) (and when will that happen??) >> + */ >> +#define LINE_SIZE (((BUFFER_SIZE)*3) + \ >> + ((((int)BUFFER_SIZE/4)+1)*2) + 3) >> + > The formatting of this is rather unusual, > > #define LINE_SIZE (BUFFER_SIZE * 3 + (BUFFER_SIZE / 4 + 1) * 2 + 3) Yup, It seems absolutely correct. I really don't know what I was thinking when I made that stupid (over) calculation error :( . I have tried running the code with your suggested modification and it seems to be working correctly even for larger and smaller values of BUFFER_SIZE (than 16). > > would be preferable. I'd fix that and apply the patch if it was the only > issue, but taking a closer look at your calculation I'm afraid I fail to > understand it. IMHO, it should be: > > for (i = 0; i < BUFFER_SIZE; i++) { > if (i < cnt) > lp += sprintf(lp, " %02x", buf[i]); > else > lp += sprintf(lp, " "); > > => 3 chars / per byte * BUFFER_SIZE, no matter if we're in the if or in > the else branch > > if ((i % 4) == 3) > lp += sprintf(lp, " |"); > > => + 2 chars after each 4th byte of BUFFER_SIZE > > if (i >= cnt || !isprint(buf[i])) > buf[i] = ' '; > } > > log_debug(level, "%s %.16s |", line, buf); > > => + '\0' termination, of course, so: > > #define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) > > , or am I missing something? Nope, It was me who made the mistake. Sincere apologies for wasting your precious time. Do you want me to re-post the complete patch for this change in a separate email ? I think I will as I have changed the comments above the #define LINE_SIZE as well to fix the explanation. In case it is not required, please ignore that. One more thing, just for my clarification. I always thought that the coding guidelines suggested tabs-stops between each component of a #define. Like: #define<tab>LINE_SIZE<tab><description of define>. Is it OK to use <space> in place of <tab> in case the line extends beyond 80 char column width to preserve readability? Sincere apologies for the blunder and thanks a lot for your effort in reviewing the patch. ---- Shreyansh |
From: Shreyansh J. <shr...@gm...> - 2008-09-03 07:51:23
|
Hi List, As discussed in this thread, following (updated with comments) patch fixes a buffer overrun problem in __dump_line function in log.c file. It also removes a stray 'return' call in log_pdu function which was restricting a PDU dump call to complete log_pdu operation. In dump_line, the array line[] was defined as line[53] whereas sprintf was pushing in 59 characters into it. This was leading to segfault of ietd when debugging was enabled. Through this patch, the size of line char array would be linked to the size of buffer defined in log_pdu function. The calculation used is (N*3 + ((N/4)*2 + 1), where N is the expected size of buffer used in log_pdu function. Also, in log_pdu function, the first check for log_level against passed parameter for log was incorrect and would have forced a return even when the caller has set the logging level to acceptable level. Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> Shreyansh ---BEGIN--- Index: usr/log.c =================================================================== --- usr/log.c (revision 1) +++ usr/log.c (working copy) @@ -62,15 +62,25 @@ } } +/* Definition for log_pdu buffer */ +#define BUFFER_SIZE 8 +/* Buffer line would be printed in the following format in syslog + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' + * Means, 57 Characters if size of line is 16 chars. For N being the buffer + * size, the fomula would be = (N*3 + ((N/4)*2 + 1), where, the middle term + * is to counter printing " |" after each 4 bytes. + */ +#define LINE_SIZE ((BUFFER_SIZE * 3) + (BUFFER_SIZE / 4) * 2 + 1) + static void __dump_line(int level, unsigned char *buf, int *cp) { - char line[16*3+5], *lp = line; + char line[LINE_SIZE], *lp = line; int i, cnt; cnt = *cp; if (!cnt) return; - for (i = 0; i < 16; i++) { + for (i = 0; i < BUFFER_SIZE; i++) { if (i < cnt) lp += sprintf(lp, " %02x", buf[i]); else @@ -89,7 +99,7 @@ int cnt = (*cp)++; buf[cnt] = ch; - if (cnt == 15) + if (cnt == (BUFFER_SIZE - 1)) __dump_line(level, buf, cp); } @@ -98,13 +108,12 @@ void log_pdu(int level, struct PDU *pdu) { - unsigned char char_buf[16]; + unsigned char char_buf[BUFFER_SIZE]; int char_cnt = 0; unsigned char *buf; int i; - return; - if (log_level <= level) + if (log_level < level) return; buf = (void *)&pdu->bhs; ---END--- |
From: Arne R. <ag...@po...> - 2008-09-04 19:20:44
|
Hi Shreyansh, I merged your patch with a few modifications - rev. 169. Below's the version I applied, hope you're ok with it. Thanks, Arne --- From: Shreyansh Jain <shrey.linux at gmail.com> Subject: [Patch 1/1] Segfault in ietd buffer overrun in dump_line Fix a buffer overrun problem in __dump_line function in log.c file. It also removes a stray 'return' call in log_pdu function which was restricting a PDU dump call to complete log_pdu operation. Also, in log_pdu function, the first check for log_level against passed parameter for log was incorrect and would have forced a return even when the caller has set the logging level to acceptable level. Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> --- usr/log.c | 24 +++++++++++++++++------- 1 files changed, 17 insertions(+), 7 deletions(-) diff --git a/usr/log.c b/usr/log.c index e5b178f..c90f7ef 100644 --- a/usr/log.c +++ b/usr/log.c @@ -62,15 +62,24 @@ void log_debug(int level, const char *fmt, ...) } } +/* Definition for log_pdu buffer */ +#define BUFFER_SIZE 16 + +/* + * size required for a hex dump of BUFFER_SIZE bytes (' ' + 2 chars = 3 chars + * per byte) with a ' |' separator each 4th byte: + */ +#define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) + static void __dump_line(int level, unsigned char *buf, int *cp) { - char line[16*3+5], *lp = line; + char line[LINE_SIZE], *lp = line; int i, cnt; cnt = *cp; if (!cnt) return; - for (i = 0; i < 16; i++) { + for (i = 0; i < BUFFER_SIZE; i++) { if (i < cnt) lp += sprintf(lp, " %02x", buf[i]); else @@ -80,7 +89,9 @@ static void __dump_line(int level, unsigned char *buf, int *cp) if (i >= cnt || !isprint(buf[i])) buf[i] = ' '; } - log_debug(level, "%s %.16s |", line, buf); + + /* buf is not \0-terminated! */ + log_debug(level, "%s %.*s |", line, BUFFER_SIZE, buf); *cp = 0; } @@ -89,7 +100,7 @@ static void __dump_char(int level, unsigned char *buf, int *cp, int ch) int cnt = (*cp)++; buf[cnt] = ch; - if (cnt == 15) + if (cnt == BUFFER_SIZE - 1) __dump_line(level, buf, cp); } @@ -98,13 +109,12 @@ static void __dump_char(int level, unsigned char *buf, int *cp, int ch) void log_pdu(int level, struct PDU *pdu) { - unsigned char char_buf[16]; + unsigned char char_buf[BUFFER_SIZE]; int char_cnt = 0; unsigned char *buf; int i; - return; - if (log_level <= level) + if (log_level < level) return; buf = (void *)&pdu->bhs; -- 1.5.4.3 |
From: Ross S. W. W. <RW...@me...> - 2008-09-04 19:30:21
|
Arne Redlich wrote: > > Hi Shreyansh, > > I merged your patch with a few modifications - rev. 169. > Below's the version I applied, hope you're ok with it. > ... > +#define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) Arne, wouldn't it be wise to parenthesis the operations then rely on the maintainers astute understanding of the order of operations? -Ross ______________________________________________________________________ This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged and/or confidential information. If you are not the intended recipient of this e-mail, you are hereby notified that any dissemination, distribution or copying of this e-mail, and any attachments thereto, is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender and permanently delete the original and any copy or printout thereof. |
From: Shreyansh J. <shr...@gm...> - 2008-09-05 08:32:44
|
Hi Arne and List, On Fri, Sep 5, 2008 at 12:50 AM, Arne Redlich <ag...@po...> wrote: > Hi Shreyansh, > > I merged your patch with a few modifications - rev. 169. > Below's the version I applied, hope you're ok with it. Thanks a lot for your help and support. I just have one doubt regarding the change, but no objections as such. [snip] > +#define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) [snip] While writing #define, I (personally) prefer enclosing sub expansions into brackets. I am not so very well versed with the way gcc behaves, but with my little experience I have come to honour the age old way of enclosing sub-expansions into brackets to avoid any problem related to compile time and mathematical calculations. Though, I agree that the above expression may not be so complex to create such problems. Nevertheless, I tried compiling and all seems fine. Thanks for your time and effort in changing the patch. [snip] > + > + /* buf is not \0-terminated! */ > + log_debug(level, "%s %.*s |", line, BUFFER_SIZE, buf); > *cp = 0; > } [snip] Thanks to you, I know one more thing about variable argument list : '%.*s' - didn't know this earlier. I always used static numbers for string length limiters. Regards, Shreyansh |
From: Arne R. <ag...@po...> - 2008-09-05 19:16:41
|
Hi Shreyansh, Am Freitag, den 05.09.2008, 14:02 +0530 schrieb Shreyansh Jain: > Hi Arne and List, > > On Fri, Sep 5, 2008 at 12:50 AM, Arne Redlich <ag...@po...> wrote: > > Hi Shreyansh, > > > > I merged your patch with a few modifications - rev. 169. > > Below's the version I applied, hope you're ok with it. > > Thanks a lot for your help and support. I just have one doubt > regarding the change, but no objections as such. > > [snip] > > +#define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) > [snip] > > While writing #define, I (personally) prefer enclosing sub expansions > into brackets. I am not so very well versed with the way gcc behaves, > but with my little experience I have come to honour the age old way of > enclosing sub-expansions into brackets to avoid any problem related to > compile time and mathematical calculations. Though, I agree that the > above expression may not be so complex to create such problems. Please note that your version does *not* protect you against these bad surprises that might be caused by, say, #define BUFFER_SIZE 10 + 6. It merely visually reinforces C's operator precedence and associativity, which I didn't think was that counter-intuitive that the parentheses were required. The correct way would obviously be to use (BUFFER_SIZE) on each occurence. So there's only a very subjective "readability" difference between your version and mine - obviously you and others (hello Ross :)) find the parenthesized version easier to read, while I'm distracted by too many levels of parentheses. HTH, Arne |
From: Ross S. W. W. <RW...@me...> - 2008-09-05 19:39:17
|
Arne Redlich wrote: > > Hi Shreyansh, > > Am Freitag, den 05.09.2008, 14:02 +0530 schrieb Shreyansh Jain: > > Hi Arne and List, > > > > On Fri, Sep 5, 2008 at 12:50 AM, Arne Redlich <ag...@po...> wrote: > > > Hi Shreyansh, > > > > > > I merged your patch with a few modifications - rev. 169. > > > Below's the version I applied, hope you're ok with it. > > > > Thanks a lot for your help and support. I just have one doubt > > regarding the change, but no objections as such. > > > > [snip] > > > +#define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) > > [snip] > > > > While writing #define, I (personally) prefer enclosing sub expansions > > into brackets. I am not so very well versed with the way gcc behaves, > > but with my little experience I have come to honour the age old way of > > enclosing sub-expansions into brackets to avoid any problem related to > > compile time and mathematical calculations. Though, I agree that the > > above expression may not be so complex to create such problems. > > Please note that your version does *not* protect you against these bad > surprises that might be caused by, say, #define BUFFER_SIZE 10 + 6. It > merely visually reinforces C's operator precedence and associativity, > which I didn't think was that counter-intuitive that the parentheses > were required. The correct way would obviously be to use (BUFFER_SIZE) > on each occurence. > > So there's only a very subjective "readability" difference between your > version and mine - obviously you and others (hello Ross :)) find the > parenthesized version easier to read, while I'm distracted by too many > levels of parentheses. I agree that over-use of parentheses is distracting and counter productive to code readability, but limited use of parentheses can actually be self-documenting. Take our little define here: #define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) This was determined to be: 1) 3 chars a byte 2) 2 chars every fourth byte 3) 1 terminating char Which presented as: #define LINE_SIZE ((BUFFER_SIZE * 3) + (BUFFER_SIZE / 4 * 2) + 1) Would help the reader to visualize the calculation IMHO, but to each their own and this is a very subjective argument. -Ross ______________________________________________________________________ This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged and/or confidential information. If you are not the intended recipient of this e-mail, you are hereby notified that any dissemination, distribution or copying of this e-mail, and any attachments thereto, is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender and permanently delete the original and any copy or printout thereof. |
From: Shreyansh J. <shr...@gm...> - 2008-09-07 16:52:19
|
Hi Arne, Ross and List, >> >> So there's only a very subjective "readability" difference between your >> version and mine - obviously you and others (hello Ross :)) find the >> parenthesized version easier to read, while I'm distracted by too many >> levels of parentheses. > > I agree that over-use of parentheses is distracting and counter > productive to code readability, but limited use of parentheses > can actually be self-documenting. > > Take our little define here: > > #define LINE_SIZE (BUFFER_SIZE * 3 + BUFFER_SIZE / 4 * 2 + 1) > > This was determined to be: > 1) 3 chars a byte > 2) 2 chars every fourth byte > 3) 1 terminating char > > Which presented as: > > #define LINE_SIZE ((BUFFER_SIZE * 3) + (BUFFER_SIZE / 4 * 2) + 1) > > Would help the reader to visualize the calculation IMHO, but > to each their own and this is a very subjective argument. I actually would like to side with Ross's view that parenthesis increase the readability of the code - but, its also true that this argument is quite subjective and dependent on individual view-points. What matters here is the code correctness (primary) and all other things are secondary. I think Iorking 'fine' (once again, fine is a subjective argument of what correct code is defined as). would like to leave the decision upon Arne (which he has already taken) as it seems to be w Arne: Please feel free to modify the code as you (the maintainer) deems correct. I have absolutely no issues with it (nothing on which I can put forward an object argument). Thanks for you help. -- Shreyansh |
From: Ross S. W. W. <RW...@me...> - 2008-08-27 14:21:30
|
Shreyansh Jain wrote: > > Hi List, > > Following patch fixes a buffer overrun problem in __dump_line function > in log.c file. It also removes a stray 'return' call in log_pdu > function which was restricting a PDU dump call to complete log_pdu > operation. > > In dump_line, the array line[] was defined as line[53] whereas sprintf > was pushing in 59 characters into it. This was leading to segfault of > ietd when debugging was enabled. Is 59 characters future proof? Maybe a DEFINE is needed to make sure the buffer size used in the sprintf and the line size defined in dump_line always matches. Can the log_pdu handle any PDU sent at it? If the code has been bypassed it means it probably hasn't been maintained, which means other problems may lurk in there. > Also, in log_pdu function, the first check for log_level against > passed parameter for log was incorrect and would have forced a return > even when the caller has set the logging level to acceptable level. I would make sure that all calls to log_pdu are free and clear too after we know the log_pdu and it's related calls have been fully bug checked. -Ross ______________________________________________________________________ This e-mail, and any attachments thereto, is intended only for use by the addressee(s) named herein and may contain legally privileged and/or confidential information. If you are not the intended recipient of this e-mail, you are hereby notified that any dissemination, distribution or copying of this e-mail, and any attachments thereto, is strictly prohibited. If you have received this e-mail in error, please immediately notify the sender and permanently delete the original and any copy or printout thereof. |
From: Shreyansh <shr...@gm...> - 2008-08-28 10:32:17
|
Hi Ross and List, Ross S. W. Walker <RWalker <at> medallion.com> writes: > > Shreyansh Jain wrote: > > > > Hi List, > > > > Following patch fixes a buffer overrun problem in __dump_line function > > in log.c file. It also removes a stray 'return' call in log_pdu > > function which was restricting a PDU dump call to complete log_pdu > > operation. > > > > In dump_line, the array line[] was defined as line[53] whereas sprintf > > was pushing in 59 characters into it. This was leading to segfault of > > ietd when debugging was enabled. > > Is 59 characters future proof? This array is part of the __dump_line function which in turn gets a 16byte array to print in a hex-dump-style format. ------8<------ ... 73 for (i = 0; i < 16; i++) { 74 if (i < cnt) 75 lp += sprintf(lp, " %02x", buf[i]); /* 16*3 */ 76 else 77 lp += sprintf(lp, " "); /* 3 chars */ 78 if ((i % 4) == 3) 79 lp += sprintf(lp, " |"); /* 4*2 */ ... ------8<------ [log.c - please ignore line numbers] That explains that this array would be used in a loop of 16, which would push a maximum of (3*16 chars (space+ 2 hex chars) + 3 + 4*2 chars) through its run (worst case). Thus, 59 seems to be a safe value. > > Maybe a DEFINE is needed to make sure the buffer size used in the > sprintf and the line size defined in dump_line always matches. Yup, this does make sense. I will do that and push the change again. > > Can the log_pdu handle any PDU sent at it? As of now, 4 places are calling log_pdu(), each passing response or request header to be printed. ------8<------ ietd.c event_loop log_pdu(2, &conn->req); iscsid.c cmnd_execute log_pdu(2, &conn->rsp); iscsid.c cmnd_execute log_pdu(2, &conn->rsp); iscsid.c cmnd_execute log_pdu(2, &conn->rsp); ------8<------ And when the PDU are sent, they are broken into set of 16bytes each using the dump_char, __dump_char set of function and then thrown to dump_line to print on screen. -----8<---- ... ... ... (in __dump_char) 92 if (cnt == 15) 93 __dump_line(level, buf, cp); ... ... (in log_pdu) 112 for (i = 0; i < BHS_SIZE; i++) 113 dump_char(*buf++); 114 dump_line(); ... -----8<---- [log.c - please ignore the line numbers] Thus, I feel if log_pdu is able to do its work of looping properly, dump_line would be able to handle its content/input well. > > If the code has been bypassed it means it probably hasn't been > maintained, which means other problems may lurk in there. Maybe, but I have been using this debugging quite heavily since last couple of days after fixing this bug and it seems to work fine. I will have a look if there are any more loose ends. > > > Also, in log_pdu function, the first check for log_level against > > passed parameter for log was incorrect and would have forced a return > > even when the caller has set the logging level to acceptable level. > > I would make sure that all calls to log_pdu are free and clear too > after we know the log_pdu and it's related calls have been fully > bug checked. log_pdu function is based on PDU structure, so, any change in this structure would certainly warrant a change in log_pdu - but till then, log_pdu should be safe for all its calls as far as I can say (and see). Thanks for the comments/suggestion. I will update the code accordingly and post back. -- Shreyansh |
From: Shreyansh <shr...@gm...> - 2008-08-28 12:39:06
|
Hi list, There indeed was a problem with the previous patch. >This array is part of the __dump_line function which in turn gets a 16byte >array to print in a hex-dump-style format. > > ------8<------ > ... > 73 for (i = 0; i < 16; i++) { > 74 if (i < cnt) > 75 lp += sprintf(lp, " %02x", buf[i]); /* 16*3 */ > 76 else > 77 lp += sprintf(lp, " "); /* 3 chars */ > 78 if ((i % 4) == 3) > 79 lp += sprintf(lp, " |"); /* 4*2 */ >... >------8<------ >[log.c - please ignore line numbers] >That explains that this array would be used in a loop of 16, which would push >a maximum of (3*16 chars (space+ 2 hex chars) + 3 + 4*2 chars) through its >run (worst case). Thus, 59 seems to be a safe value. I had calculated the size of array to be 59, which should have been 61. But, the part of code which is in else condition of "if(i < cnt)" (i.e. p += sprintf(lp, " "); ) is not reachable in this for loop and hence 59 was working fine. I do not find any condition in which i==16 and still be in the for loop. Am I wrong? Nevertheless, I have mailed across another patch which incorporated comments from Ross for making the size of line dependent on buffer. In case the else condition should be removed from that code, please let me know. -- Shreyansh -- View this message in context: http://www.nabble.com/-Patch-1-1--Segfault-in-ietd-buffer-overrun-in-dump_line-tp19175885p19199688.html Sent from the iscsitarget-devel mailing list archive at Nabble.com. |
From: Shreyansh J. <shr...@gm...> - 2008-08-28 12:21:07
|
Hi, Following patch fixes a buffer overrun problem in __dump_line function in log.c file. It also removes a stray 'return' call in log_pdu function which was restricting a PDU dump call to complete log_pdu operation. In dump_line, the array line[] was defined as line[53] whereas sprintf was pushing in 59 characters into it. This was leading to segfault of ietd when debugging was enabled. Through this patch, the size of line char array would be linked to the size of buffer defined in log_pdu function. The calculation used is (N*3 + ((N/4)+1)*2 + 3), where N is the expected size of buffer used in log_pdu function. Also, in log_pdu function, the first check for log_level against passed parameter for log was incorrect and would have forced a return even when the caller has set the logging level to acceptable level. --- I have verified the patch by forcing the four entry points of this function (in ietd.c and iscsid.c) to use this function, and also by modifying the size of the buffer being used. In case someone feels that more testcases may have been left out, please let me know. All others comments/suggestions are also welcome. --- Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> --------BEGIN--------- Index: usr/log.c =================================================================== --- usr/log.c (revision 168) +++ usr/log.c (working copy) @@ -62,15 +62,26 @@ } } +/* Definition for log_pdu buffer */ +#define BUFFER_SIZE 16 +/* Buffer line would be printed in the following format in syslog + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' + * Means, 61 Characters if size of line is 16 chars. For N being the buffer + * size, the fomula would be = (N*3 + ((N/4)+1)*2 + 3), where, the middle term + * is to counter printing " | " after each 4 bytes. Last '3' is to compensate + * for " " printed when (i >= cnt) (and when will that happen??) + */ +#define LINE_SIZE (((BUFFER_SIZE)*3) + ((((int)BUFFER_SIZE/4)+1)*2) + 3) + static void __dump_line(int level, unsigned char *buf, int *cp) { - char line[16*3+5], *lp = line; + char line[LINE_SIZE], *lp = line; int i, cnt; cnt = *cp; if (!cnt) return; - for (i = 0; i < 16; i++) { + for (i = 0; i < BUFFER_SIZE; i++) { if (i < cnt) lp += sprintf(lp, " %02x", buf[i]); else @@ -89,7 +100,7 @@ int cnt = (*cp)++; buf[cnt] = ch; - if (cnt == 15) + if (cnt == (BUFFER_SIZE - 1)) __dump_line(level, buf, cp); } @@ -98,13 +109,12 @@ void log_pdu(int level, struct PDU *pdu) { - unsigned char char_buf[16]; + unsigned char char_buf[BUFFER_SIZE]; int char_cnt = 0; unsigned char *buf; int i; - return; - if (log_level <= level) + if (log_level < level) return; buf = (void *)&pdu->bhs; --------END--------- |
From: Arne R. <ag...@po...> - 2008-08-29 15:59:06
|
Am Donnerstag, den 28.08.2008, 17:51 +0530 schrieb Shreyansh Jain: > Hi, > > Following patch fixes a buffer overrun problem in __dump_line function > in log.c file. It also removes a stray 'return' call in log_pdu > function which was restricting a PDU dump call to complete log_pdu operation. > > In dump_line, the array line[] was defined as line[53] whereas sprintf > was pushing in 59 characters into it. This was leading to segfault of > ietd when debugging was enabled. Through this patch, the size of line > char array would be linked to the size of buffer defined in log_pdu > function. The calculation used is (N*3 + ((N/4)+1)*2 + 3), where N is > the expected size of buffer used in log_pdu function. > > Also, in log_pdu function, the first check for log_level against > passed parameter for log was incorrect and would have forced a return > even when the caller has set the logging level to acceptable level. > > --- > I have verified the patch by forcing the four entry points of this > function (in ietd.c and iscsid.c) to use this function, and also by > modifying the size of the buffer being used. In case someone feels > that more testcases may have been left out, please let me know. All > others comments/suggestions are also welcome. > --- > Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com> Hi Shreyansh, thanks a lot for the patch. However, from a first glance it seems the formatting is broken, e.g. the LINE_SIZE define is on multiple lines and tabs were replaced with space chars. Please see if you can fix your MUA (send a patch to yourself and try to apply it). I'll take a closer look at your actual code modifications in the next few days. Thanks again, Arne Oh, there's no need for the BEGIN / END tags. :) > --------BEGIN--------- > Index: usr/log.c > =================================================================== > --- usr/log.c (revision 168) > +++ usr/log.c (working copy) > @@ -62,15 +62,26 @@ > } > } > > +/* Definition for log_pdu buffer */ > +#define BUFFER_SIZE 16 > +/* Buffer line would be printed in the following format in syslog > + * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |' > + * Means, 61 Characters if size of line is 16 chars. For N being the buffer > + * size, the fomula would be = (N*3 + ((N/4)+1)*2 + 3), where, the middle term > + * is to counter printing " | " after each 4 bytes. Last '3' is to compensate > + * for " " printed when (i >= cnt) (and when will that happen??) > + */ > +#define LINE_SIZE (((BUFFER_SIZE)*3) + > ((((int)BUFFER_SIZE/4)+1)*2) + 3) > + > static void __dump_line(int level, unsigned char *buf, int *cp) > { > - char line[16*3+5], *lp = line; > + char line[LINE_SIZE], *lp = line; > int i, cnt; > > cnt = *cp; > if (!cnt) > return; > - for (i = 0; i < 16; i++) { > + for (i = 0; i < BUFFER_SIZE; i++) { > if (i < cnt) > lp += sprintf(lp, " %02x", buf[i]); > else > @@ -89,7 +100,7 @@ > int cnt = (*cp)++; > > buf[cnt] = ch; > - if (cnt == 15) > + if (cnt == (BUFFER_SIZE - 1)) > __dump_line(level, buf, cp); > } > > @@ -98,13 +109,12 @@ > > void log_pdu(int level, struct PDU *pdu) > { > - unsigned char char_buf[16]; > + unsigned char char_buf[BUFFER_SIZE]; > int char_cnt = 0; > unsigned char *buf; > int i; > - return; > > - if (log_level <= level) > + if (log_level < level) > return; > > buf = (void *)&pdu->bhs; > > --------END--------- > > ------------------------------------------------------------------------- > This SF.Net email is sponsored by the Moblin Your Move Developer's challenge > Build the coolest Linux based applications with Moblin SDK & win great prizes > Grand prize is a trip for two to an Open Source event anywhere in the world > http://moblin-contest.org/redirect.php?banner_id=100&url=/ > _______________________________________________ > Iscsitarget-devel mailing list > Isc...@li... > https://lists.sourceforge.net/lists/listinfo/iscsitarget-devel > |
From: Shreyansh J. <shr...@gm...> - 2008-08-29 17:22:43
|
Hi Arne and List, On Fri, Aug 29, 2008 at 9:15 PM, Arne Redlich <ag...@po...> wrote: > Am Donnerstag, den 28.08.2008, 17:51 +0530 schrieb Shreyansh Jain: >> Hi, [snip] > thanks a lot for the patch. However, from a first glance it seems the > formatting is broken, e.g. the LINE_SIZE define is on multiple lines and > tabs were replaced with space chars. Please see if you can fix your MUA > (send a patch to yourself and try to apply it). > > I'll take a closer look at your actual code modifications in the next > few days. > > Thanks again, [snip] >> + */ >> +#define LINE_SIZE (((BUFFER_SIZE)*3) + >> ((((int)BUFFER_SIZE/4)+1)*2) + 3) >> + Yup, I did see that the #define was breaking. Actually I noticed before pasting that this line was ending at the 79th character (in vim) and hence thought this might be Ok (assuming that lines should end within 80char limit) Currently I have limited access to web, so I will re-send it on Monday with proper alignment. Thanks a lot for your comments. Regards, Shreyansh |