libkmod: Handle long lines in /proc/modules
diff mbox

Message ID 1466172255-20955-1-git-send-email-mmarek@suse.com
State Accepted
Headers show

Commit Message

Michal Marek June 17, 2016, 2:04 p.m. UTC
From: Michal Marek <mmarek@suse.cz>

kmod_module_new_from_loaded() calls fgets with a 4k buffer. When a
module such as usbcore is used by too many modules, the rest of the line
is considered a beginning of another lines and we eventually get errors
like these from lsmod:

libkmod: kmod_module_get_holders: could not open '/sys/module/100,/holders': No such file or directory

together with bogus entries in the output. In kmod_module_get_size, the
problem does not affect functionality, but the line numbers in error
messages will be wrong.

Signed-off-by: Michal Marek <mmarek@suse.com>
---

I wrote a test case for this as well, but it is failing because the
testsuite itself has problems with output larger than 4k. I'll send
something later.

Also, the buffer could be shrinked now, so that we do not use that much
space on stack. Not sure if this is of interest or not. I left it as is.

Michal


---
 libkmod/libkmod-module.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Lucas De Marchi June 20, 2016, 12:55 p.m. UTC | #1
Hi Michal,

On Fri, Jun 17, 2016 at 11:04 AM, Michal Marek <mmarek@suse.com> wrote:
> From: Michal Marek <mmarek@suse.cz>
>
> kmod_module_new_from_loaded() calls fgets with a 4k buffer. When a
> module such as usbcore is used by too many modules, the rest of the line
> is considered a beginning of another lines and we eventually get errors
> like these from lsmod:
>
> libkmod: kmod_module_get_holders: could not open '/sys/module/100,/holders': No such file or directory
>
> together with bogus entries in the output. In kmod_module_get_size, the
> problem does not affect functionality, but the line numbers in error
> messages will be wrong.

ugh, thanks for catching this.

>
> Signed-off-by: Michal Marek <mmarek@suse.com>
> ---
>
> I wrote a test case for this as well, but it is failing because the
> testsuite itself has problems with output larger than 4k. I'll send
> something later.
>
> Also, the buffer could be shrinked now, so that we do not use that much
> space on stack. Not sure if this is of interest or not. I left it as is.

Yep, I think it's reasonable to be 64 bytes since it's the maximum
length of a module name

> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 1460c6746cc4..25dcda7667b7 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -1660,13 +1660,14 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>                 struct kmod_module *m;
>                 struct kmod_list *node;
>                 int err;
> +               size_t len = strlen(line);
>                 char *saveptr, *name = strtok_r(line, " \t", &saveptr);
>
>                 err = kmod_module_new_from_name(ctx, name, &m);
>                 if (err < 0) {
>                         ERR(ctx, "could not get module from name '%s': %s\n",
>                                 name, strerror(-err));
> -                       continue;
> +                       goto eat_line;

I think it would be better to "eat line" before anything else, above
kmod_module_new_from_name(). So you don't need to change the flow
here.

>                 }
>
>                 node = kmod_list_append(l, m);
> @@ -1676,6 +1677,9 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>                         ERR(ctx, "out of memory\n");
>                         kmod_module_unref(m);
>                 }
> +eat_line:
> +               while (line[len - 1] != '\n' && fgets(line, sizeof(line), fp))
> +                       len = strlen(line);
>         }
>
>         fclose(fp);
> @@ -1825,12 +1829,13 @@ KMOD_EXPORT long kmod_module_get_size(const struct kmod_module *mod)
>         }
>
>         while (fgets(line, sizeof(line), fp)) {
> +               size_t len = strlen(line);
>                 char *saveptr, *endptr, *tok = strtok_r(line, " \t", &saveptr);
>                 long value;
>
>                 lineno++;
>                 if (tok == NULL || !streq(tok, mod->name))
> -                       continue;
> +                       goto eat_line;

same thing here.

thanks
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek June 20, 2016, 1:16 p.m. UTC | #2
On 2016-06-20 14:55, Lucas De Marchi wrote:
> On Fri, Jun 17, 2016 at 11:04 AM, Michal Marek <mmarek@suse.com> wrote:
>> Also, the buffer could be shrinked now, so that we do not use that much
>> space on stack. Not sure if this is of interest or not. I left it as is.
> 
> Yep, I think it's reasonable to be 64 bytes since it's the maximum
> length of a module name

OK. In kmod_module_get_size(), the buffer needs to be larger, though.


>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 1460c6746cc4..25dcda7667b7 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -1660,13 +1660,14 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>>                 struct kmod_module *m;
>>                 struct kmod_list *node;
>>                 int err;
>> +               size_t len = strlen(line);
>>                 char *saveptr, *name = strtok_r(line, " \t", &saveptr);
>>
>>                 err = kmod_module_new_from_name(ctx, name, &m);
>>                 if (err < 0) {
>>                         ERR(ctx, "could not get module from name '%s': %s\n",
>>                                 name, strerror(-err));
>> -                       continue;
>> +                       goto eat_line;
> 
> I think it would be better to "eat line" before anything else, above
> kmod_module_new_from_name(). So you don't need to change the flow
> here.

Sure, but then we would need two buffers, because name is a pointer into
line.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas De Marchi June 20, 2016, 1:41 p.m. UTC | #3
On Mon, Jun 20, 2016 at 10:16 AM, Michal Marek <mmarek@suse.com> wrote:
> On 2016-06-20 14:55, Lucas De Marchi wrote:
>> On Fri, Jun 17, 2016 at 11:04 AM, Michal Marek <mmarek@suse.com> wrote:
>>> Also, the buffer could be shrinked now, so that we do not use that much
>>> space on stack. Not sure if this is of interest or not. I left it as is.
>>
>> Yep, I think it's reasonable to be 64 bytes since it's the maximum
>> length of a module name
>
> OK. In kmod_module_get_size(), the buffer needs to be larger, though.
>
>
>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>> index 1460c6746cc4..25dcda7667b7 100644
>>> --- a/libkmod/libkmod-module.c
>>> +++ b/libkmod/libkmod-module.c
>>> @@ -1660,13 +1660,14 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>>>                 struct kmod_module *m;
>>>                 struct kmod_list *node;
>>>                 int err;
>>> +               size_t len = strlen(line);
>>>                 char *saveptr, *name = strtok_r(line, " \t", &saveptr);
>>>
>>>                 err = kmod_module_new_from_name(ctx, name, &m);
>>>                 if (err < 0) {
>>>                         ERR(ctx, "could not get module from name '%s': %s\n",
>>>                                 name, strerror(-err));
>>> -                       continue;
>>> +                       goto eat_line;
>>
>> I think it would be better to "eat line" before anything else, above
>> kmod_module_new_from_name(). So you don't need to change the flow
>> here.
>
> Sure, but then we would need two buffers, because name is a pointer into
> line.

oh, right. Thinking again, I'm ok with your version.

Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek June 20, 2016, 2:54 p.m. UTC | #4
On 2016-06-20 15:41, Lucas De Marchi wrote:
> On Mon, Jun 20, 2016 at 10:16 AM, Michal Marek <mmarek@suse.com> wrote:
>> On 2016-06-20 14:55, Lucas De Marchi wrote:
>>> On Fri, Jun 17, 2016 at 11:04 AM, Michal Marek <mmarek@suse.com> wrote:
>>>> Also, the buffer could be shrinked now, so that we do not use that much
>>>> space on stack. Not sure if this is of interest or not. I left it as is.
>>>
>>> Yep, I think it's reasonable to be 64 bytes since it's the maximum
>>> length of a module name
>>
>> OK. In kmod_module_get_size(), the buffer needs to be larger, though.
>>
>>
>>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>>> index 1460c6746cc4..25dcda7667b7 100644
>>>> --- a/libkmod/libkmod-module.c
>>>> +++ b/libkmod/libkmod-module.c
>>>> @@ -1660,13 +1660,14 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>>>>                 struct kmod_module *m;
>>>>                 struct kmod_list *node;
>>>>                 int err;
>>>> +               size_t len = strlen(line);
>>>>                 char *saveptr, *name = strtok_r(line, " \t", &saveptr);
>>>>
>>>>                 err = kmod_module_new_from_name(ctx, name, &m);
>>>>                 if (err < 0) {
>>>>                         ERR(ctx, "could not get module from name '%s': %s\n",
>>>>                                 name, strerror(-err));
>>>> -                       continue;
>>>> +                       goto eat_line;
>>>
>>> I think it would be better to "eat line" before anything else, above
>>> kmod_module_new_from_name(). So you don't need to change the flow
>>> here.
>>
>> Sure, but then we would need two buffers, because name is a pointer into
>> line.
> 
> oh, right. Thinking again, I'm ok with your version.

Shall I post a version that reduces the size of the buffer, or are you
going to apply my patch as is?

Thanks,
Michal

--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas De Marchi June 21, 2016, 9:06 p.m. UTC | #5
On Mon, Jun 20, 2016 at 11:54 AM, Michal Marek <mmarek@suse.com> wrote:
> On 2016-06-20 15:41, Lucas De Marchi wrote:
>> On Mon, Jun 20, 2016 at 10:16 AM, Michal Marek <mmarek@suse.com> wrote:
>>> On 2016-06-20 14:55, Lucas De Marchi wrote:
>>>> On Fri, Jun 17, 2016 at 11:04 AM, Michal Marek <mmarek@suse.com> wrote:
>>>>> Also, the buffer could be shrinked now, so that we do not use that much
>>>>> space on stack. Not sure if this is of interest or not. I left it as is.
>>>>
>>>> Yep, I think it's reasonable to be 64 bytes since it's the maximum
>>>> length of a module name
>>>
>>> OK. In kmod_module_get_size(), the buffer needs to be larger, though.
>>>
>>>
>>>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>>>> index 1460c6746cc4..25dcda7667b7 100644
>>>>> --- a/libkmod/libkmod-module.c
>>>>> +++ b/libkmod/libkmod-module.c
>>>>> @@ -1660,13 +1660,14 @@ KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
>>>>>                 struct kmod_module *m;
>>>>>                 struct kmod_list *node;
>>>>>                 int err;
>>>>> +               size_t len = strlen(line);
>>>>>                 char *saveptr, *name = strtok_r(line, " \t", &saveptr);
>>>>>
>>>>>                 err = kmod_module_new_from_name(ctx, name, &m);
>>>>>                 if (err < 0) {
>>>>>                         ERR(ctx, "could not get module from name '%s': %s\n",
>>>>>                                 name, strerror(-err));
>>>>> -                       continue;
>>>>> +                       goto eat_line;
>>>>
>>>> I think it would be better to "eat line" before anything else, above
>>>> kmod_module_new_from_name(). So you don't need to change the flow
>>>> here.
>>>
>>> Sure, but then we would need two buffers, because name is a pointer into
>>> line.
>>
>> oh, right. Thinking again, I'm ok with your version.
>
> Shall I post a version that reduces the size of the buffer, or are you
> going to apply my patch as is?

Applied

thanks
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 1460c6746cc4..25dcda7667b7 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -1660,13 +1660,14 @@  KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
 		struct kmod_module *m;
 		struct kmod_list *node;
 		int err;
+		size_t len = strlen(line);
 		char *saveptr, *name = strtok_r(line, " \t", &saveptr);
 
 		err = kmod_module_new_from_name(ctx, name, &m);
 		if (err < 0) {
 			ERR(ctx, "could not get module from name '%s': %s\n",
 				name, strerror(-err));
-			continue;
+			goto eat_line;
 		}
 
 		node = kmod_list_append(l, m);
@@ -1676,6 +1677,9 @@  KMOD_EXPORT int kmod_module_new_from_loaded(struct kmod_ctx *ctx,
 			ERR(ctx, "out of memory\n");
 			kmod_module_unref(m);
 		}
+eat_line:
+		while (line[len - 1] != '\n' && fgets(line, sizeof(line), fp))
+			len = strlen(line);
 	}
 
 	fclose(fp);
@@ -1825,12 +1829,13 @@  KMOD_EXPORT long kmod_module_get_size(const struct kmod_module *mod)
 	}
 
 	while (fgets(line, sizeof(line), fp)) {
+		size_t len = strlen(line);
 		char *saveptr, *endptr, *tok = strtok_r(line, " \t", &saveptr);
 		long value;
 
 		lineno++;
 		if (tok == NULL || !streq(tok, mod->name))
-			continue;
+			goto eat_line;
 
 		tok = strtok_r(NULL, " \t", &saveptr);
 		if (tok == NULL) {
@@ -1848,6 +1853,9 @@  KMOD_EXPORT long kmod_module_get_size(const struct kmod_module *mod)
 
 		size = value;
 		break;
+eat_line:
+		while (line[len - 1] != '\n' && fgets(line, sizeof(line), fp))
+			len = strlen(line);
 	}
 	fclose(fp);