Re: [PATCH lttng-tools 1/2] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

Re: [PATCH lttng-tools 1/2] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Mathieu Desnoyers]

----- On Mar 13, 2017, at 6:11 PM, Jonathan Rajotte jonathan.rajotte-julien@efficios.com wrote:

> fscanf on an empty file returns directly without assigning value to
> 'index' leading to assigning the value of an uninitialized variable to
> syscall_table_nb_entry. This can result in memory allocation problems
> when listing syscalls on 'lttng list --kernel --syscall'[1][2].
> 
> Fixes #1091
> 
> [1] https://bugs.lttng.org/issues/1091
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
> src/bin/lttng-sessiond/syscall.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
> index 6ee38bd..c21e4d8 100644
> --- a/src/bin/lttng-sessiond/syscall.c
> +++ b/src/bin/lttng-sessiond/syscall.c
> @@ -16,6 +16,8 @@
>  */
> 
> #define _LGPL_SOURCE
> +#include <stdbool.h>
> +
> #include <common/bitfield.h>
> #include <common/common.h>
> #include <common/kernel-ctl/kernel-ctl.h>
> @@ -43,7 +45,8 @@ int syscall_init_table(void)
> 	size_t nbmem;
> 	FILE *fp;
> 	/* Syscall data from the kernel. */
> -	size_t index;
> +	size_t index = 0;
> +	bool at_least_one_syscall = false;
> 	uint32_t bitness;
> 	char name[SYSCALL_NAME_LEN];
> 
> @@ -76,7 +79,8 @@ int syscall_init_table(void)
> 				name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
> 				bitness = %u; };\n",
> 				&index, name, &bitness) == 3) {
> -		if (index >= nbmem ) {
> +		at_least_one_syscall = true;
> +		if (index >= nbmem) {
> 			struct syscall *new_list;
> 			size_t new_nbmem;
> 
> @@ -123,7 +127,10 @@ int syscall_init_table(void)
> 		*/
> 	}
> 
> -	syscall_table_nb_entry = index;
> +	/* Index start at 0. */

start -> starts

other than that:

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> +	if (at_least_one_syscall) {
> +		syscall_table_nb_entry = index + 1;
> +	}
> 
> 	ret = 0;
> 
> --
> 2.7.4
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools 1/2] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Mathieu Desnoyers]

----- On Mar 13, 2017, at 9:12 PM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Mar 13, 2017, at 6:11 PM, Jonathan Rajotte
> jonathan.rajotte-julien@efficios.com wrote:
> 
>> fscanf on an empty file returns directly without assigning value to
>> 'index' leading to assigning the value of an uninitialized variable to
>> syscall_table_nb_entry. This can result in memory allocation problems
>> when listing syscalls on 'lttng list --kernel --syscall'[1][2].

Actually, you should also state in this changelog that it fixes
a second issue: we lose the last element due to off-by-one on
index.

Thanks,

Mathieu

>> 
>> Fixes #1091
>> 
>> [1] https://bugs.lttng.org/issues/1091
>> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/
>> 
>> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
>> ---
>> src/bin/lttng-sessiond/syscall.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
>> index 6ee38bd..c21e4d8 100644
>> --- a/src/bin/lttng-sessiond/syscall.c
>> +++ b/src/bin/lttng-sessiond/syscall.c
>> @@ -16,6 +16,8 @@
>>  */
>> 
>> #define _LGPL_SOURCE
>> +#include <stdbool.h>
>> +
>> #include <common/bitfield.h>
>> #include <common/common.h>
>> #include <common/kernel-ctl/kernel-ctl.h>
>> @@ -43,7 +45,8 @@ int syscall_init_table(void)
>> 	size_t nbmem;
>> 	FILE *fp;
>> 	/* Syscall data from the kernel. */
>> -	size_t index;
>> +	size_t index = 0;
>> +	bool at_least_one_syscall = false;
>> 	uint32_t bitness;
>> 	char name[SYSCALL_NAME_LEN];
>> 
>> @@ -76,7 +79,8 @@ int syscall_init_table(void)
>> 				name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
>> 				bitness = %u; };\n",
>> 				&index, name, &bitness) == 3) {
>> -		if (index >= nbmem ) {
>> +		at_least_one_syscall = true;
>> +		if (index >= nbmem) {
>> 			struct syscall *new_list;
>> 			size_t new_nbmem;
>> 
>> @@ -123,7 +127,10 @@ int syscall_init_table(void)
>> 		*/
>> 	}
>> 
>> -	syscall_table_nb_entry = index;
>> +	/* Index start at 0. */
> 
> start -> starts
> 
> other than that:
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
>> +	if (at_least_one_syscall) {
>> +		syscall_table_nb_entry = index + 1;
>> +	}
>> 
>> 	ret = 0;
>> 
>> --
>> 2.7.4
>> 
>> _______________________________________________
>> lttng-dev mailing list
>> lttng-dev@lists.lttng.org
>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[PATCH lttng-tools v2] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Jonathan Rajotte]

v2: merged previous series in one patch since the second patch is a minor fix
and partly addressed in the first patch.

"start" -> starts

--

fscanf on an empty file returns directly without assigning value to
'index' leading to assigning the value of an uninitialized variable to
syscall_table_nb_entry. This can result in memory allocation problems
when listing syscalls on 'lttng list --kernel --syscall'[1][2].

Fixes at the same time an off-by-one problem for the index
value.

The index value returned by fscanf is an index starting at 0.
However, it is later used to count how many elements need to be
allocated, without any extra increment.

It does not cause issues in practice because SYSCALL_TABLE_INIT_SIZE
is nonzero, and because we don't require the table to expand by more
than the double of its size at once (which could happen if we could
have a hole in the syscall table for instance).

It's better to fix this off-by-one for clarity, and in case this code
gets re-used in other contexts that don't have the same memory growth
pattern.

Fixes #1091

[1] https://bugs.lttng.org/issues/1091
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/bin/lttng-sessiond/syscall.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
index 6ee38bd..7d0a92b 100644
--- a/src/bin/lttng-sessiond/syscall.c
+++ b/src/bin/lttng-sessiond/syscall.c
@@ -16,6 +16,8 @@
  */
 
 #define _LGPL_SOURCE
+#include <stdbool.h>
+
 #include <common/bitfield.h>
 #include <common/common.h>
 #include <common/kernel-ctl/kernel-ctl.h>
@@ -43,7 +45,8 @@ int syscall_init_table(void)
 	size_t nbmem;
 	FILE *fp;
 	/* Syscall data from the kernel. */
-	size_t index;
+	size_t index = 0;
+	bool at_least_one_syscall = false;
 	uint32_t bitness;
 	char name[SYSCALL_NAME_LEN];
 
@@ -76,12 +79,13 @@ int syscall_init_table(void)
 				name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
 				bitness = %u; };\n",
 				&index, name, &bitness) == 3) {
-		if (index >= nbmem ) {
+		at_least_one_syscall = true;
+		if (index >= nbmem) {
 			struct syscall *new_list;
 			size_t new_nbmem;
 
 			/* Double memory size. */
-			new_nbmem = max(index, nbmem << 1);
+			new_nbmem = max(index + 1, nbmem << 1);
 			if (new_nbmem > (SIZE_MAX / sizeof(*new_list))) {
 				/* Overflow, stop everything, something went really wrong. */
 				ERR("Syscall listing memory size overflow. Stopping");
@@ -123,7 +127,10 @@ int syscall_init_table(void)
 		*/
 	}
 
-	syscall_table_nb_entry = index;
+	/* Index starts at 0. */
+	if (at_least_one_syscall) {
+		syscall_table_nb_entry = index + 1;
+	}
 
 	ret = 0;
 
-- 
2.7.4

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools v2] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Mathieu Desnoyers]

----- On Mar 14, 2017, at 10:37 AM, Jonathan Rajotte jonathan.rajotte-julien@efficios.com wrote:

> v2: merged previous series in one patch since the second patch is a minor fix
> and partly addressed in the first patch.
> 
> "start" -> starts
> 
> --
> 
> fscanf on an empty file returns directly without assigning value to
> 'index' leading to assigning the value of an uninitialized variable to
> syscall_table_nb_entry. This can result in memory allocation problems
> when listing syscalls on 'lttng list --kernel --syscall'[1][2].
> 
> Fixes at the same time an off-by-one problem for the index
> value.

Fixing this off-by-one also fixes the missing last element issue. It should
be described in this changelog, but it's not. Only the effect on memory
allocation is discussed.

Thanks,

Mathieu

> 
> The index value returned by fscanf is an index starting at 0.
> However, it is later used to count how many elements need to be
> allocated, without any extra increment.
> 
> It does not cause issues in practice because SYSCALL_TABLE_INIT_SIZE
> is nonzero, and because we don't require the table to expand by more
> than the double of its size at once (which could happen if we could
> have a hole in the syscall table for instance).
> 
> It's better to fix this off-by-one for clarity, and in case this code
> gets re-used in other contexts that don't have the same memory growth
> pattern.
> 
> Fixes #1091
> 
> [1] https://bugs.lttng.org/issues/1091
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/
> 
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
> src/bin/lttng-sessiond/syscall.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
> index 6ee38bd..7d0a92b 100644
> --- a/src/bin/lttng-sessiond/syscall.c
> +++ b/src/bin/lttng-sessiond/syscall.c
> @@ -16,6 +16,8 @@
>  */
> 
> #define _LGPL_SOURCE
> +#include <stdbool.h>
> +
> #include <common/bitfield.h>
> #include <common/common.h>
> #include <common/kernel-ctl/kernel-ctl.h>
> @@ -43,7 +45,8 @@ int syscall_init_table(void)
> 	size_t nbmem;
> 	FILE *fp;
> 	/* Syscall data from the kernel. */
> -	size_t index;
> +	size_t index = 0;
> +	bool at_least_one_syscall = false;
> 	uint32_t bitness;
> 	char name[SYSCALL_NAME_LEN];
> 
> @@ -76,12 +79,13 @@ int syscall_init_table(void)
> 				name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
> 				bitness = %u; };\n",
> 				&index, name, &bitness) == 3) {
> -		if (index >= nbmem ) {
> +		at_least_one_syscall = true;
> +		if (index >= nbmem) {
> 			struct syscall *new_list;
> 			size_t new_nbmem;
> 
> 			/* Double memory size. */
> -			new_nbmem = max(index, nbmem << 1);
> +			new_nbmem = max(index + 1, nbmem << 1);
> 			if (new_nbmem > (SIZE_MAX / sizeof(*new_list))) {
> 				/* Overflow, stop everything, something went really wrong. */
> 				ERR("Syscall listing memory size overflow. Stopping");
> @@ -123,7 +127,10 @@ int syscall_init_table(void)
> 		*/
> 	}
> 
> -	syscall_table_nb_entry = index;
> +	/* Index starts at 0. */
> +	if (at_least_one_syscall) {
> +		syscall_table_nb_entry = index + 1;
> +	}
> 
> 	ret = 0;
> 
> --
> 2.7.4
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

[PATCH lttng-tools v3] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Jonathan Rajotte]

v3: change commit message to include information regarding off-by-one
problems induced by 'index' and the use of 'index' as value of
syscall_table_nb_entry.

--

fscanf on an empty file returns directly without assigning value to
'index' leading to assigning the value of an uninitialized variable to
syscall_table_nb_entry. This can result in memory allocation problems
when listing syscalls on 'lttng list --kernel --syscall'[1][2].

Fixes at the same time an off-by-one problem for the
syscall_table_nb_entry value and an off-by-one error on table memory
reallocation.

The index value returned by fscanf is an index starting at 0. It is
later assigned to syscall_table_nb_entry which is used for memory
allocation and iteration during syscall_table_list. Forgetting to add 1
results in losing the last syscall during listing.

The parsed index value is also used to count how many elements should be
allocated during table reallocation, without any extra increment which
result in an off-by-one error. Hence, make sure to increment its value by
one when assigning the value of syscall_table_nb_entry. It does not
cause issues in practice because SYSCALL_TABLE_INIT_SIZE is nonzero, and
because we don't require the table to expand by more than the double of
its size at once (which could happen if we could have a hole in the
syscall table for instance).

Fixes #1091

[1] https://bugs.lttng.org/issues/1091
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/

Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
---
 src/bin/lttng-sessiond/syscall.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
index 6ee38bd..7d0a92b 100644
--- a/src/bin/lttng-sessiond/syscall.c
+++ b/src/bin/lttng-sessiond/syscall.c
@@ -16,6 +16,8 @@
  */
 
 #define _LGPL_SOURCE
+#include <stdbool.h>
+
 #include <common/bitfield.h>
 #include <common/common.h>
 #include <common/kernel-ctl/kernel-ctl.h>
@@ -43,7 +45,8 @@ int syscall_init_table(void)
 	size_t nbmem;
 	FILE *fp;
 	/* Syscall data from the kernel. */
-	size_t index;
+	size_t index = 0;
+	bool at_least_one_syscall = false;
 	uint32_t bitness;
 	char name[SYSCALL_NAME_LEN];
 
@@ -76,12 +79,13 @@ int syscall_init_table(void)
 				name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
 				bitness = %u; };\n",
 				&index, name, &bitness) == 3) {
-		if (index >= nbmem ) {
+		at_least_one_syscall = true;
+		if (index >= nbmem) {
 			struct syscall *new_list;
 			size_t new_nbmem;
 
 			/* Double memory size. */
-			new_nbmem = max(index, nbmem << 1);
+			new_nbmem = max(index + 1, nbmem << 1);
 			if (new_nbmem > (SIZE_MAX / sizeof(*new_list))) {
 				/* Overflow, stop everything, something went really wrong. */
 				ERR("Syscall listing memory size overflow. Stopping");
@@ -123,7 +127,10 @@ int syscall_init_table(void)
 		*/
 	}
 
-	syscall_table_nb_entry = index;
+	/* Index starts at 0. */
+	if (at_least_one_syscall) {
+		syscall_table_nb_entry = index + 1;
+	}
 
 	ret = 0;
 
-- 
2.7.4

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

Re: [PATCH lttng-tools v3] Fix: syscall_table_nb_entry invalid value when no syscalls TPs are defined

[Jérémie Galarneau]

Merged, thanks!

Jérémie

On 15 March 2017 at 10:59, Jonathan Rajotte
<jonathan.rajotte-julien@efficios.com> wrote:
> v3: change commit message to include information regarding off-by-one
> problems induced by 'index' and the use of 'index' as value of
> syscall_table_nb_entry.
>
> --
>
> fscanf on an empty file returns directly without assigning value to
> 'index' leading to assigning the value of an uninitialized variable to
> syscall_table_nb_entry. This can result in memory allocation problems
> when listing syscalls on 'lttng list --kernel --syscall'[1][2].
>
> Fixes at the same time an off-by-one problem for the
> syscall_table_nb_entry value and an off-by-one error on table memory
> reallocation.
>
> The index value returned by fscanf is an index starting at 0. It is
> later assigned to syscall_table_nb_entry which is used for memory
> allocation and iteration during syscall_table_list. Forgetting to add 1
> results in losing the last syscall during listing.
>
> The parsed index value is also used to count how many elements should be
> allocated during table reallocation, without any extra increment which
> result in an off-by-one error. Hence, make sure to increment its value by
> one when assigning the value of syscall_table_nb_entry. It does not
> cause issues in practice because SYSCALL_TABLE_INIT_SIZE is nonzero, and
> because we don't require the table to expand by more than the double of
> its size at once (which could happen if we could have a hole in the
> syscall table for instance).
>
> Fixes #1091
>
> [1] https://bugs.lttng.org/issues/1091
> [2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1671063/
>
> Signed-off-by: Jonathan Rajotte <jonathan.rajotte-julien@efficios.com>
> ---
>  src/bin/lttng-sessiond/syscall.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/bin/lttng-sessiond/syscall.c b/src/bin/lttng-sessiond/syscall.c
> index 6ee38bd..7d0a92b 100644
> --- a/src/bin/lttng-sessiond/syscall.c
> +++ b/src/bin/lttng-sessiond/syscall.c
> @@ -16,6 +16,8 @@
>   */
>
>  #define _LGPL_SOURCE
> +#include <stdbool.h>
> +
>  #include <common/bitfield.h>
>  #include <common/common.h>
>  #include <common/kernel-ctl/kernel-ctl.h>
> @@ -43,7 +45,8 @@ int syscall_init_table(void)
>         size_t nbmem;
>         FILE *fp;
>         /* Syscall data from the kernel. */
> -       size_t index;
> +       size_t index = 0;
> +       bool at_least_one_syscall = false;
>         uint32_t bitness;
>         char name[SYSCALL_NAME_LEN];
>
> @@ -76,12 +79,13 @@ int syscall_init_table(void)
>                                 name = %" XSTR(SYSCALL_NAME_LEN) "[^;]; \
>                                 bitness = %u; };\n",
>                                 &index, name, &bitness) == 3) {
> -               if (index >= nbmem ) {
> +               at_least_one_syscall = true;
> +               if (index >= nbmem) {
>                         struct syscall *new_list;
>                         size_t new_nbmem;
>
>                         /* Double memory size. */
> -                       new_nbmem = max(index, nbmem << 1);
> +                       new_nbmem = max(index + 1, nbmem << 1);
>                         if (new_nbmem > (SIZE_MAX / sizeof(*new_list))) {
>                                 /* Overflow, stop everything, something went really wrong. */
>                                 ERR("Syscall listing memory size overflow. Stopping");
> @@ -123,7 +127,10 @@ int syscall_init_table(void)
>                 */
>         }
>
> -       syscall_table_nb_entry = index;
> +       /* Index starts at 0. */
> +       if (at_least_one_syscall) {
> +               syscall_table_nb_entry = index + 1;
> +       }
>
>         ret = 0;
>
> --
> 2.7.4
>



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev