[PATCH XENSTORE v1 00/10] Code analysis fixes

Norbert Manthey posted 10 patches 3 years, 1 month ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210226144144.9252-1-nmanthey@amazon.de
tools/libs/store/xs.c            | 10 +++++++++-
tools/xenstore/xenstore_client.c |  3 +++
tools/xenstore/xenstored_core.c  | 16 ++++++++++++++++
tools/xenstore/xenstored_core.h  |  2 +-
tools/xenstore/xenstored_posix.c |  6 +++++-
tools/xenstore/xs_tdb_dump.c     |  6 +++---
6 files changed, 37 insertions(+), 6 deletions(-)
[PATCH XENSTORE v1 00/10] Code analysis fixes
Posted by Norbert Manthey 3 years, 1 month ago
Dear all,

we have been running some code analysis tools on the xenstore code, and triaged
the results. This series presents the robustness fixes we identified.

Best,
Norbert

Michael Kurth (1):
  xenstore: add missing NULL check

Norbert Manthey (9):
  xenstore: add missing NULL check
  xenstore: fix print format string
  xenstore: check formats of trace
  xenstore_client: handle memory on error
  xenstore: handle daemon creation errors
  xenstored: handle port reads correctly
  xenstore: handle do_mkdir and do_rm failure
  xs: handle daemon socket error
  xs: add error handling

 tools/libs/store/xs.c            | 10 +++++++++-
 tools/xenstore/xenstore_client.c |  3 +++
 tools/xenstore/xenstored_core.c  | 16 ++++++++++++++++
 tools/xenstore/xenstored_core.h  |  2 +-
 tools/xenstore/xenstored_posix.c |  6 +++++-
 tools/xenstore/xs_tdb_dump.c     |  6 +++---
 6 files changed, 37 insertions(+), 6 deletions(-)

-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
Posted by Julien Grall 3 years, 1 month ago
Hi Norbert,

Thank you for the patches. Please find below a state for each patches.

On 26/02/2021 14:41, Norbert Manthey wrote:

For the following patches:

>    xenstore: add missing NULL check
>    xenstore: fix print format string
 >    xenstore: check formats of trace
 >    xenstore: handle do_mkdir and do_rm failure
 >    xenstore: add missing NULL check
 >    xs: add error handling

They are fully reviewed and Ian provided a release-acked-by. So I have 
merged them to staging.

Note that last one was merged with the commit message/title tweaked.

For the following patches:

>    xenstore_client: handle memory on error
>    xenstore: handle daemon creation errors

They are fully reviewed but so far Ian didn't provided a 
release-acked-by. If you (or someone else) think they should be merged, 
then please reply on each patch.

For now, I have merged them to my for-next/4.16 branch [1]. The patches 
will be folded in staging when the tree is re-opened.

For the following patch:

>    xenstored: handle port reads correctly

IIUC, this was dropped.

For the following patch:

>    xs: handle daemon socket error

Ian had one question about it. I haven't committed it in any branch for now.

Cheers,

[1] 
https://xenbits.xen.org/gitweb/?p=people/julieng/xen-unstable.git;a=shortlog;h=refs/heads/for-next/4.16

-- 
Julien Grall

[for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
Posted by Julien Grall 3 years, 1 month ago
Hi,

I have tagged the e-mail with 4.15 as I think we likely want some of the 
patches to be in the next release.

As a minimum, we get the following:
   - patch #7: xenstore: handle do_mkdir and do_rm failure
   - patch #8: xenstore: add missing NULL check
   - patch #10: xs: add error handling

The first two add missing NULL check in runtime code in XenStored. The 
3rd one adds a missing NULL check in xs_is_domain_introduced() in 
libxenstore (can be used at runtime by xenpaging at least).

In addition to that, I would like to consider patch #3: xenstore: check 
formats of trace. It is allowing the compiler to check the format printf 
for trace(). This should be low-risk.

For the rest is a mix of silencing coverity and potential errors either 
at init or in a standalone binaries.

The init ones would be useful (patch #1, #5, #9) for Xenstored 
LiveUpdate as they would be potential triggered when upgrading the 
binary. But I am not sure whether we consider LiveUpdate supported.

Any thoughts?

Cheers,

On 26/02/2021 14:41, Norbert Manthey wrote:
> Dear all,
> 
> we have been running some code analysis tools on the xenstore code, and triaged
> the results. This series presents the robustness fixes we identified.
> 
> Best,
> Norbert
> 
> Michael Kurth (1):
>    xenstore: add missing NULL check
> 
> Norbert Manthey (9):
>    xenstore: add missing NULL check
>    xenstore: fix print format string
>    xenstore: check formats of trace
>    xenstore_client: handle memory on error
>    xenstore: handle daemon creation errors
>    xenstored: handle port reads correctly
>    xenstore: handle do_mkdir and do_rm failure
>    xs: handle daemon socket error
>    xs: add error handling
> 
>   tools/libs/store/xs.c            | 10 +++++++++-
>   tools/xenstore/xenstore_client.c |  3 +++
>   tools/xenstore/xenstored_core.c  | 16 ++++++++++++++++
>   tools/xenstore/xenstored_core.h  |  2 +-
>   tools/xenstore/xenstored_posix.c |  6 +++++-
>   tools/xenstore/xs_tdb_dump.c     |  6 +++---
>   6 files changed, 37 insertions(+), 6 deletions(-)
> 

-- 
Julien Grall

Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
Posted by Andrew Cooper 3 years, 1 month ago
On 01/03/2021 18:01, Julien Grall wrote:
> Hi,
>
> I have tagged the e-mail with 4.15 as I think we likely want some of
> the patches to be in the next release.
>
> As a minimum, we get the following:
>   - patch #7: xenstore: handle do_mkdir and do_rm failure
>   - patch #8: xenstore: add missing NULL check
>   - patch #10: xs: add error handling
>
> The first two add missing NULL check in runtime code in XenStored. The
> 3rd one adds a missing NULL check in xs_is_domain_introduced() in
> libxenstore (can be used at runtime by xenpaging at least).
>
> In addition to that, I would like to consider patch #3: xenstore:
> check formats of trace. It is allowing the compiler to check the
> format printf for trace(). This should be low-risk.
>
> For the rest is a mix of silencing coverity and potential errors
> either at init or in a standalone binaries.
>
> The init ones would be useful (patch #1, #5, #9) for Xenstored
> LiveUpdate as they would be potential triggered when upgrading the
> binary. But I am not sure whether we consider LiveUpdate supported.
>
> Any thoughts?

Live Update is a headline feature, even if only tech preview.

I'd say that all bugfixes are fair game, and low risk.  All these fixes
(other than the evtchn one which has an outstanding question) look to be
reasonable to take.  They're all simple and obvious fixes pointed out by
static analysis.

~Andrew

Re: [for-4.15] Re: [PATCH XENSTORE v1 00/10] Code analysis fixes
Posted by Julien Grall 3 years, 1 month ago
Hi Andrew,

On 01/03/2021 19:39, Andrew Cooper wrote:
> On 01/03/2021 18:01, Julien Grall wrote:
>> Hi,
>>
>> I have tagged the e-mail with 4.15 as I think we likely want some of
>> the patches to be in the next release.
>>
>> As a minimum, we get the following:
>>    - patch #7: xenstore: handle do_mkdir and do_rm failure
>>    - patch #8: xenstore: add missing NULL check
>>    - patch #10: xs: add error handling
>>
>> The first two add missing NULL check in runtime code in XenStored. The
>> 3rd one adds a missing NULL check in xs_is_domain_introduced() in
>> libxenstore (can be used at runtime by xenpaging at least).
>>
>> In addition to that, I would like to consider patch #3: xenstore:
>> check formats of trace. It is allowing the compiler to check the
>> format printf for trace(). This should be low-risk.
>>
>> For the rest is a mix of silencing coverity and potential errors
>> either at init or in a standalone binaries.
>>
>> The init ones would be useful (patch #1, #5, #9) for Xenstored
>> LiveUpdate as they would be potential triggered when upgrading the
>> binary. But I am not sure whether we consider LiveUpdate supported.
>>
>> Any thoughts?
> 
> Live Update is a headline feature, even if only tech preview.

I thought it was a tech preview but I couldn't find the statement in 
SUPPORT.MD. I guess we should update it before 4.15 is released.

> 
> I'd say that all bugfixes are fair game, and low risk.  All these fixes
> (other than the evtchn one which has an outstanding question) look to be
> reasonable to take.  They're all simple and obvious fixes pointed out by
> static analysis.

That's a fair point. I wanted to set an order as I know the rules are 
getting tighter. So this gives an opportunity to Ian to have enough data 
to decide what's the best.

Cheers,

-- 
Julien Grall