Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Gedare Bloom]

On Mon, Mar 15, 2021 at 5:14 PM Chris Johns  wrote:
>
> On 16/3/21 10:07 am, Joel Sherrill wrote:
> > On Mon, Mar 15, 2021 at 6:01 PM Chris Johns  > > wrote:
> > On 16/3/21 6:55 am, Joel Sherrill wrote:
> > >
> > >
> > > On Mon, Mar 15, 2021 at 2:46 PM Gedare Bloom  > 
> > > >> wrote:
> > >
> > > On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  > 
> > > >> wrote:
> > > >
> > > > On 13/3/21 2:18 am, Ryan Long wrote:
> > > > > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> > > > >
> > > > > Closes #4296
> > > > > ---
> > > > >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/cpukit/libmisc/shell/hexdump-parse.c
> > > b/cpukit/libmisc/shell/hexdump-parse.c
> > > > > index 88b9d56..5b56bbf 100644
> > > > > --- a/cpukit/libmisc/shell/hexdump-parse.c
> > > > > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> > > > > @@ -462,6 +462,9 @@ isint2:
> > >  switch(fu->bcnt) {
> > > > >   (void)printf("\n");
> > > > >   }
> > > > >  #endif
> > > > > +#ifdef __rtems__
> > > > > + free(nextpr);
> > > > > +#endif
> > > >
> > > > I know this has not been done in imported code in rtems.git 
> > before but
> > > should we
> > > > adopt the libbsd standard of adding /* __rtems__ */ to the 
> > #else and
> > #endif?
> > >
> > > Probably, but we also clone-and-own this shell/ code, and we 
> > should
> > > not bother with these #ifdefs in there. I think I have said this 3
> > > times this past week about shell/. The upstream does not want our
> > > changes, and we don't import from them anymore.
> > >
> > > Some of these files have massive changes and some don't. Ryan and
> > > I looked at main_cp.c and it had at least 40 revisions since the 
> > version
> > > we have. The same Coverity issue appeared to be present but the 
> > variable
> > > names were changed and much clearer.
> >
> > Yes.
> >
> > > Chris imported this code initially. I'll trust his ruling on this 
> > since I
> > assume
> > > he is likely to either be the one to update it eventually or have to 
> > help
> > someone
> > > a lot. :)
> >
> > If the code was updated I would use the libbsd way of handing it. It 
> > has been
> > proven to work.
> >
> > > And some of it is close enough that the ifdef's are worth it. Some 
> > isn't.
> > > Hard call on the overall value.
> >
> > For the existing code it is hard to call. If I was starting again I 
> > would say we
> > had to support a clean separation.
> >
> >
> > I assume you would checkout the original version and diff it. But would the
> > ifdef rtems ease the burden any?
>
> Yes and a good question, I do not know. I suppose it would depend on the file
> and changes.
>
> > I tend to like going with them otherwise, we have some files we do it
> > on and others we don't.
>
> This makes sense so if we feel it would help even now lets add them and if we
> add then I feel it is only small amount of extra effort to add them in the
> libbsd style.
>

It is good to set consistent rules at any rate.

> Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Chris Johns]

On 16/3/21 10:07 am, Joel Sherrill wrote:
> On Mon, Mar 15, 2021 at 6:01 PM Chris Johns  > wrote:
> On 16/3/21 6:55 am, Joel Sherrill wrote:
> >
> >
> > On Mon, Mar 15, 2021 at 2:46 PM Gedare Bloom  
> > >> wrote:
> >
> >     On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  
> >     >> wrote:
> >     >
> >     > On 13/3/21 2:18 am, Ryan Long wrote:
> >     > > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> >     > >
> >     > > Closes #4296
> >     > > ---
> >     > >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> >     > >  1 file changed, 3 insertions(+)
> >     > >
> >     > > diff --git a/cpukit/libmisc/shell/hexdump-parse.c
> >     b/cpukit/libmisc/shell/hexdump-parse.c
> >     > > index 88b9d56..5b56bbf 100644
> >     > > --- a/cpukit/libmisc/shell/hexdump-parse.c
> >     > > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> >     > > @@ -462,6 +462,9 @@ isint2:                                 
> >      switch(fu->bcnt) {
> >     > >               (void)printf("\n");
> >     > >       }
> >     > >  #endif
> >     > > +#ifdef __rtems__
> >     > > +     free(nextpr);
> >     > > +#endif
> >     >
> >     > I know this has not been done in imported code in rtems.git 
> before but
> >     should we
> >     > adopt the libbsd standard of adding /* __rtems__ */ to the #else 
> and
> #endif?
> >
> >     Probably, but we also clone-and-own this shell/ code, and we should
> >     not bother with these #ifdefs in there. I think I have said this 3
> >     times this past week about shell/. The upstream does not want our
> >     changes, and we don't import from them anymore.
> >
> > Some of these files have massive changes and some don't. Ryan and 
> > I looked at main_cp.c and it had at least 40 revisions since the version
> > we have. The same Coverity issue appeared to be present but the variable
> > names were changed and much clearer.
> 
> Yes.
> 
> > Chris imported this code initially. I'll trust his ruling on this since 
> I
> assume
> > he is likely to either be the one to update it eventually or have to 
> help
> someone
> > a lot. :)
> 
> If the code was updated I would use the libbsd way of handing it. It has 
> been
> proven to work.
> 
> > And some of it is close enough that the ifdef's are worth it. Some 
> isn't. 
> > Hard call on the overall value.
> 
> For the existing code it is hard to call. If I was starting again I would 
> say we
> had to support a clean separation.
> 
> 
> I assume you would checkout the original version and diff it. But would the
> ifdef rtems ease the burden any?

Yes and a good question, I do not know. I suppose it would depend on the file
and changes.

> I tend to like going with them otherwise, we have some files we do it
> on and others we don't.

This makes sense so if we feel it would help even now lets add them and if we
add then I feel it is only small amount of extra effort to add them in the
libbsd style.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Joel Sherrill]

On Mon, Mar 15, 2021 at 6:01 PM Chris Johns  wrote:

>
>
> On 16/3/21 6:55 am, Joel Sherrill wrote:
> >
> >
> > On Mon, Mar 15, 2021 at 2:46 PM Gedare Bloom  > > wrote:
> >
> > On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  > > wrote:
> > >
> > > On 13/3/21 2:18 am, Ryan Long wrote:
> > > > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> > > >
> > > > Closes #4296
> > > > ---
> > > >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/cpukit/libmisc/shell/hexdump-parse.c
> > b/cpukit/libmisc/shell/hexdump-parse.c
> > > > index 88b9d56..5b56bbf 100644
> > > > --- a/cpukit/libmisc/shell/hexdump-parse.c
> > > > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> > > > @@ -462,6 +462,9 @@ isint2:
> >  switch(fu->bcnt) {
> > > >   (void)printf("\n");
> > > >   }
> > > >  #endif
> > > > +#ifdef __rtems__
> > > > + free(nextpr);
> > > > +#endif
> > >
> > > I know this has not been done in imported code in rtems.git before
> but
> > should we
> > > adopt the libbsd standard of adding /* __rtems__ */ to the #else
> and #endif?
> >
> > Probably, but we also clone-and-own this shell/ code, and we should
> > not bother with these #ifdefs in there. I think I have said this 3
> > times this past week about shell/. The upstream does not want our
> > changes, and we don't import from them anymore.
> >
> > Some of these files have massive changes and some don't. Ryan and
> > I looked at main_cp.c and it had at least 40 revisions since the version
> > we have. The same Coverity issue appeared to be present but the variable
> > names were changed and much clearer.
>
> Yes.
>
> > Chris imported this code initially. I'll trust his ruling on this since
> I assume
> > he is likely to either be the one to update it eventually or have to
> help someone
> > a lot. :)
>
> If the code was updated I would use the libbsd way of handing it. It has
> been
> proven to work.
>
> > And some of it is close enough that the ifdef's are worth it. Some
> isn't.
> > Hard call on the overall value.
>
> For the existing code it is hard to call. If I was starting again I would
> say we
> had to support a clean separation.
>

I assume you would checkout the original version and diff it. But would the
ifdef rtems ease the burden any?

I tend to like going with them otherwise, we have some files we do it
on and others we don't.

--joel

>
> Chris
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Chris Johns]

On 16/3/21 6:55 am, Joel Sherrill wrote:
> 
> 
> On Mon, Mar 15, 2021 at 2:46 PM Gedare Bloom  > wrote:
> 
> On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  > wrote:
> >
> > On 13/3/21 2:18 am, Ryan Long wrote:
> > > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> > >
> > > Closes #4296
> > > ---
> > >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/cpukit/libmisc/shell/hexdump-parse.c
> b/cpukit/libmisc/shell/hexdump-parse.c
> > > index 88b9d56..5b56bbf 100644
> > > --- a/cpukit/libmisc/shell/hexdump-parse.c
> > > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> > > @@ -462,6 +462,9 @@ isint2:                                 
>  switch(fu->bcnt) {
> > >               (void)printf("\n");
> > >       }
> > >  #endif
> > > +#ifdef __rtems__
> > > +     free(nextpr);
> > > +#endif
> >
> > I know this has not been done in imported code in rtems.git before but
> should we
> > adopt the libbsd standard of adding /* __rtems__ */ to the #else and 
> #endif?
> 
> Probably, but we also clone-and-own this shell/ code, and we should
> not bother with these #ifdefs in there. I think I have said this 3
> times this past week about shell/. The upstream does not want our
> changes, and we don't import from them anymore.
> 
> Some of these files have massive changes and some don't. Ryan and 
> I looked at main_cp.c and it had at least 40 revisions since the version
> we have. The same Coverity issue appeared to be present but the variable
> names were changed and much clearer.

Yes.

> Chris imported this code initially. I'll trust his ruling on this since I 
> assume
> he is likely to either be the one to update it eventually or have to help 
> someone
> a lot. :)

If the code was updated I would use the libbsd way of handing it. It has been
proven to work.

> And some of it is close enough that the ifdef's are worth it. Some isn't. 
> Hard call on the overall value.

For the existing code it is hard to call. If I was starting again I would say we
had to support a clean separation.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Joel Sherrill]

On Mon, Mar 15, 2021 at 2:46 PM Gedare Bloom  wrote:

> On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  wrote:
> >
> > On 13/3/21 2:18 am, Ryan Long wrote:
> > > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> > >
> > > Closes #4296
> > > ---
> > >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/cpukit/libmisc/shell/hexdump-parse.c
> b/cpukit/libmisc/shell/hexdump-parse.c
> > > index 88b9d56..5b56bbf 100644
> > > --- a/cpukit/libmisc/shell/hexdump-parse.c
> > > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> > > @@ -462,6 +462,9 @@ isint2:
>  switch(fu->bcnt) {
> > >   (void)printf("\n");
> > >   }
> > >  #endif
> > > +#ifdef __rtems__
> > > + free(nextpr);
> > > +#endif
> >
> > I know this has not been done in imported code in rtems.git before but
> should we
> > adopt the libbsd standard of adding /* __rtems__ */ to the #else and
> #endif?
> >
>
> Probably, but we also clone-and-own this shell/ code, and we should
> not bother with these #ifdefs in there. I think I have said this 3
> times this past week about shell/. The upstream does not want our
> changes, and we don't import from them anymore.
>

Some of these files have massive changes and some don't. Ryan and
I looked at main_cp.c and it had at least 40 revisions since the version
we have. The same Coverity issue appeared to be present but the variable
names were changed and much clearer.

Chris imported this code initially. I'll trust his ruling on this since I
assume
he is likely to either be the one to update it eventually or have to help
someone
a lot. :)

And some of it is close enough that the ifdef's are worth it. Some isn't.
Hard call on the overall value.

--joel


> > Chris
> > ___
> > devel mailing list
> > devel@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Gedare Bloom]

On Sun, Mar 14, 2021 at 8:27 PM Chris Johns  wrote:
>
> On 13/3/21 2:18 am, Ryan Long wrote:
> > CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> >
> > Closes #4296
> > ---
> >  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/cpukit/libmisc/shell/hexdump-parse.c 
> > b/cpukit/libmisc/shell/hexdump-parse.c
> > index 88b9d56..5b56bbf 100644
> > --- a/cpukit/libmisc/shell/hexdump-parse.c
> > +++ b/cpukit/libmisc/shell/hexdump-parse.c
> > @@ -462,6 +462,9 @@ isint2:   
> > switch(fu->bcnt) {
> >   (void)printf("\n");
> >   }
> >  #endif
> > +#ifdef __rtems__
> > + free(nextpr);
> > +#endif
>
> I know this has not been done in imported code in rtems.git before but should 
> we
> adopt the libbsd standard of adding /* __rtems__ */ to the #else and #endif?
>

Probably, but we also clone-and-own this shell/ code, and we should
not bother with these #ifdefs in there. I think I have said this 3
times this past week about shell/. The upstream does not want our
changes, and we don't import from them anymore.

> Chris
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Chris Johns]

On 13/3/21 2:18 am, Ryan Long wrote:
> CID 26032: Resource leak in rtems_shell_hexdump_rewrite().
> 
> Closes #4296
> ---
>  cpukit/libmisc/shell/hexdump-parse.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/cpukit/libmisc/shell/hexdump-parse.c 
> b/cpukit/libmisc/shell/hexdump-parse.c
> index 88b9d56..5b56bbf 100644
> --- a/cpukit/libmisc/shell/hexdump-parse.c
> +++ b/cpukit/libmisc/shell/hexdump-parse.c
> @@ -462,6 +462,9 @@ isint2:   
> switch(fu->bcnt) {
>   (void)printf("\n");
>   }
>  #endif
> +#ifdef __rtems__
> + free(nextpr);
> +#endif

I know this has not been done in imported code in rtems.git before but should we
adopt the libbsd standard of adding /* __rtems__ */ to the #else and #endif?

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

[PATCH 1/5] hexdump-parse.c: Fix Resource leak (CID #26032)

[Ryan Long]

CID 26032: Resource leak in rtems_shell_hexdump_rewrite().

Closes #4296
---
 cpukit/libmisc/shell/hexdump-parse.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cpukit/libmisc/shell/hexdump-parse.c 
b/cpukit/libmisc/shell/hexdump-parse.c
index 88b9d56..5b56bbf 100644
--- a/cpukit/libmisc/shell/hexdump-parse.c
+++ b/cpukit/libmisc/shell/hexdump-parse.c
@@ -462,6 +462,9 @@ isint2: 
switch(fu->bcnt) {
(void)printf("\n");
}
 #endif
+#ifdef __rtems__
+   free(nextpr);
+#endif
 }
 
 void
-- 
1.8.3.1

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel