[PATCH conntrack-tools 0/3] fix potential memory loss and exit codes

[PATCH conntrack-tools 0/3] fix potential memory loss and exit codes

[Donald Yandt]

Vector data will be lost if reallocation fails, leading to undefined behaviour. 
Additionally, size_t should be used to provide the indices and sizes of vector elements.

If no configuration file or an invalid parameter is provided, the daemon should exit with
a failure status.

Donald Yandt (3):
  conntrackd: prevent memory loss if reallocation fails
  conntrackd: use size_t for element indices
  conntrackd: exit with failure status

 src/main.c   | 5 ++---
 src/vector.c | 9 ++++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.44.0

[PATCH conntrack-tools 1/3] conntrackd: prevent memory loss if reallocation fails

[Donald Yandt]

Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
---
 src/vector.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/vector.c b/src/vector.c
index c81e7ce..7f9bc3c 100644
--- a/src/vector.c
+++ b/src/vector.c
@@ -62,11 +62,12 @@ int vector_add(struct vector *v, void *data)
 {
 	if (v->cur_elems >= v->max_elems) {
 		v->max_elems += DEFAULT_VECTOR_GROWTH;
-		v->data = realloc(v->data, v->max_elems * v->size);
-		if (v->data == NULL) {
+		void *ptr = realloc(v->data, v->max_elems * v->size);
+		if (ptr == NULL) {
 			v->max_elems -= DEFAULT_VECTOR_GROWTH;
 			return -1;
 		}
+		v->data = ptr;
 	}
 	memcpy(v->data + (v->size * v->cur_elems), data, v->size);
 	v->cur_elems++;
-- 
2.44.0

Re: [PATCH conntrack-tools 1/3] conntrackd: prevent memory loss if reallocation fails

[Pablo Neira Ayuso]

On Fri, Mar 01, 2024 at 12:07:29PM -0500, Donald Yandt wrote:
> Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
> ---
>  src/vector.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/vector.c b/src/vector.c
> index c81e7ce..7f9bc3c 100644
> --- a/src/vector.c
> +++ b/src/vector.c
> @@ -62,11 +62,12 @@ int vector_add(struct vector *v, void *data)
>  {
>  	if (v->cur_elems >= v->max_elems) {
>  		v->max_elems += DEFAULT_VECTOR_GROWTH;
> -		v->data = realloc(v->data, v->max_elems * v->size);
> -		if (v->data == NULL) {

Good catch.

> +		void *ptr = realloc(v->data, v->max_elems * v->size);

Could you declare void *ptr at the top of the function? Following old
style variable declarations?

Thanks.

> +		if (ptr == NULL) {
>  			v->max_elems -= DEFAULT_VECTOR_GROWTH;
>  			return -1;
>  		}
> +		v->data = ptr;
>  	}
>  	memcpy(v->data + (v->size * v->cur_elems), data, v->size);
>  	v->cur_elems++;
> -- 
> 2.44.0
> 
> 

Re: [PATCH conntrack-tools 1/3] conntrackd: prevent memory loss if reallocation fails

[Donald Yandt]

On Sat, Mar 2, 2024 at 4:54 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Could you declare void *ptr at the top of the function? Following old
> style variable declarations?
>
> Thanks.
>

I moved the variable declaration to the top of the function in v2 as suggested.

Thanks!

[PATCH conntrack-tools 2/3] conntrackd: use size_t for element indices

[Donald Yandt]

Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
---
 src/vector.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/vector.c b/src/vector.c
index 7f9bc3c..ac1f5d9 100644
--- a/src/vector.c
+++ b/src/vector.c
@@ -23,9 +23,7 @@
 
 struct vector {
 	char *data;
-	unsigned int cur_elems;
-	unsigned int max_elems;
-	size_t size;
+	size_t cur_elems, max_elems, size;
 };
 
 #define DEFAULT_VECTOR_MEMBERS	8
-- 
2.44.0

Re: [PATCH conntrack-tools 2/3] conntrackd: use size_t for element indices

[Pablo Neira Ayuso]

Hi,

Could you describe why these are needed?

Thanks!

On Fri, Mar 01, 2024 at 12:07:30PM -0500, Donald Yandt wrote:
> Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
> ---
>  src/vector.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/vector.c b/src/vector.c
> index 7f9bc3c..ac1f5d9 100644
> --- a/src/vector.c
> +++ b/src/vector.c
> @@ -23,9 +23,7 @@
>  
>  struct vector {
>  	char *data;
> -	unsigned int cur_elems;
> -	unsigned int max_elems;
> -	size_t size;
> +	size_t cur_elems, max_elems, size;
>  };
>  
>  #define DEFAULT_VECTOR_MEMBERS	8
> -- 
> 2.44.0
> 
> 

Re: [PATCH conntrack-tools 2/3] conntrackd: use size_t for element indices

[Donald Yandt]

On Sat, Mar 2, 2024 at 4:53 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi,
>
> Could you describe why these are needed?
>
> Thanks!
>

Hi Pablo,

I mentioned it briefly in the cover letter and explained why it should
be used in the commit message for version 2.
If you require any additional detail, please let me know.

Thank you

On Sat, Mar 2, 2024 at 4:53 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi,
>
> Could you describe why these are needed?
>
> Thanks!
>
> On Fri, Mar 01, 2024 at 12:07:30PM -0500, Donald Yandt wrote:
> > Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
> > ---
> >  src/vector.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/src/vector.c b/src/vector.c
> > index 7f9bc3c..ac1f5d9 100644
> > --- a/src/vector.c
> > +++ b/src/vector.c
> > @@ -23,9 +23,7 @@
> >
> >  struct vector {
> >       char *data;
> > -     unsigned int cur_elems;
> > -     unsigned int max_elems;
> > -     size_t size;
> > +     size_t cur_elems, max_elems, size;
> >  };
> >
> >  #define DEFAULT_VECTOR_MEMBERS       8
> > --
> > 2.44.0
> >
> >

Re: [PATCH conntrack-tools 2/3] conntrackd: use size_t for element indices

[Pablo Neira Ayuso]

On Sat, Mar 02, 2024 at 11:20:38AM -0500, Donald Yandt wrote:
> On Sat, Mar 2, 2024 at 4:53 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >
> > Hi,
> >
> > Could you describe why these are needed?
> >
> > Thanks!
> >
> 
> Hi Pablo,
> 
> I mentioned it briefly in the cover letter and explained why it should
> be used in the commit message for version 2.
> If you require any additional detail, please let me know.

Cover letter gets lost when applying patches.

Split descriptions into each individual commit, it really helps us.

Thanks a lot

[PATCH conntrack-tools 3/3] conntrackd: exit with failure status

[Donald Yandt]

Signed-off-by: Donald Yandt <donald.yandt@gmail.com>
---
 src/main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/main.c b/src/main.c
index de4773d..c6b2600 100644
--- a/src/main.c
+++ b/src/main.c
@@ -175,7 +175,7 @@ int main(int argc, char *argv[])
 			}
 			show_usage(argv[0]);
 			dlog(LOG_ERR, "Missing config filename");
-			break;
+			exit(EXIT_FAILURE);
 		case 'F':
 			set_operation_mode(&type, REQUEST, argv);
 			i = set_action_by_table(i, argc, argv,
@@ -309,8 +309,7 @@ int main(int argc, char *argv[])
 		default:
 			show_usage(argv[0]);
 			dlog(LOG_ERR, "Unknown option: %s", argv[i]);
-			return 0;
-			break;
+			exit(EXIT_FAILURE);
 		}
 	}
 
-- 
2.44.0