Bug in function idbm_recinfo_config()

[rahul]

Guys,

    I see few problems with the function idbm_recinfo_config(). Can someone please take a look and confirm ?

void idbm_recinfo_config(recinfo_t *info, FILE *f)

{

char name[NAME_MAXVAL];

char value[VALUE_MAXVAL];

char *line, *nl, buffer[2048];

int line_number = 0;

int c = 0, i;

fseek(f, 0, SEEK_SET);

/* process the config file */

do {

line = fgets(buffer, sizeof (buffer), f);

line_number++;

if (!line)

continue;

nl = line + strlen(line) - 1;

if (*nl != '\n') {

log_warning("Config file line %d too long.",

      line_number);

continue;

}

Here, if the line is too long that it cannot fit into the buffer, shouldn't we need to ignore the rest of the line ?

Secondly, while reading the "name", we do not check for the array size. 

/* parse name */

i=0; nl = line; *name = 0;

while (*nl && !isspace(c = *nl) && *nl != '=') {

*(name+i) = *nl; i++; nl++;               <<< we may go beyond end of array "name".

}

Similarly, we can go beyond end of array "value".

while (*nl) {

*(value+i) = *nl; i++; nl++;

}

thanks,

rahul

[manish singh]

Hi Rahul,

 

Please see inline

On Mon, Jan 21, 2013 at 12:24 PM, rahul <junky_...@yahoo.co.in> wrote:

Guys,

    I see few problems with the function idbm_recinfo_config(). Can someone please take a look and confirm ?

void idbm_recinfo_config(recinfo_t *info, FILE *f)

{

char name[NAME_MAXVAL];

char value[VALUE_MAXVAL];

char *line, *nl, buffer[2048];

int line_number = 0;

int c = 0, i;

fseek(f, 0, SEEK_SET);

/* process the config file */

do {

line = fgets(buffer, sizeof (buffer), f);

line_number++;

if (!line)

continue;

nl = line + strlen(line) - 1;

if (*nl != '\n') {

log_warning("Config file line %d too long.",

      line_number);

continue;

}

Here, if the line is too long that it cannot fit into the buffer, shouldn't we need to ignore the rest of the line ?

 

 

I dont see anything worng here fgets reads either till end of line or till given size.

Refer to link

http://pubs.opengroup.org/onlinepubs/007904875/functions/fgets.html

 

The fgets() function shall read bytes from stream into the array pointed to by s, until n-1 bytes are read, or a <newline> is read and transferred to s, or an end-of-file condition is encountered. The string is then terminated with a null byte.

 

 

Secondly, while reading the "name", we do not check for the array size. 

/* parse name */

i=0; nl = line; *name = 0;

while (*nl && !isspace(c = *nl) && *nl != '=') {

*(name+i) = *nl; i++; nl++;               <<< we may go beyond end of array "name".

}

Similarly, we can go beyond end of array "value".

while (*nl) {

*(value+i) = *nl; i++; nl++;

}

thanks,

rahul

--
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To view this discussion on the web visit https://groups.google.com/d/msg/open-iscsi/-/f3nnefBafawJ.
To post to this group, send email to open-...@googlegroups.com.
To unsubscribe from this group, send email to open-iscsi+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.

--
Regards

Manish Kumar Singh
Member Technical Staff
Chelsio (India) Private Limited
Subramanya Arcade, Floor 3, Tower B
No. 12, Bannerghatta Road, Bangalore-560076
Karnataka, India
 T: +1-91-80-4039-6800 Ext. 2234

[rahul]

The problem is that if the line is too long (> 2048 chars), then in the next iteration it will start reading after 2048 characters which doesn't look correct. If the line is longer than 2048 chars we should skip anything beyond 2048 on that line.

  

[Mike Christie]

Yeah, for really long param=settings strings then it is busted and the
code will not read in the value correctly. Continuing to read that line
will not help unless we get lucky and all those chars before the
param=setting string are whitespaces and we end up just right in the
strings and not miss any of the info in the string.

Right now, I do not think we have any setting that long though, so it
does not come up, and having tons of whitespace also does not come up.

I do not know if any one has time to fix. Is there another bug you are
thinking about (if there is a segfault or corrupter or security issue
then we should fix) or are you hitting a issue with really long lines in
a real setup, or is this one of those things where incorrect code is
just irking you and you want it fixed.

[rahul]

Thanks Mike for taking a look at this. On a real set up we might not hit these issues. I was just going through this code when I noticed this.

[Ulrich Windl]

Hi!

Simple proposal: Detect if the buffer read is completely full. The issue a warning that the following read is expected to read nonsense.

It sdoesn't fix the problem, but keeps people aware should the problem become relevant.

Weak variant: Add a comment in the source to document that.

Regards,
Ulrich


>>> Mike Christie <mich...@cs.wisc.edu> schrieb am 21.01.2013 um 22:16 in
Nachricht <50FDB03F...@cs.wisc.edu>:

[rahul]

On Tuesday, January 22, 2013 12:43:56 PM UTC+5:30, Uli wrote:

Hi!

Simple proposal: Detect if the buffer read is completely full. The issue a warning that the following read is expected to read nonsense.

It sdoesn't fix the problem, but keeps people aware should the problem become relevant.

Weak variant: Add a comment in the source to document that.

Regards,
Ulrich

We can simply ignore the rest of the line.

               if (*nl != '\n') {

                        log_warning("Config file line %d too long.",

                               line_number);

               

                        /* discard the rest of the line */

                        while((c = fgetc(f)) != '\n' && c != EOF);

                

                       continue;

                }