tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open
PATCH 1/3 tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open
From: Pramod Devendra <[email protected]>
--- tools/libxl/libxl_qmp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
tools/libxl/libxl_qmp.c +4 -1
@@ -369,10 +369,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path, ret =libxl_fd_set_cloexec (qmp->ctx, qmp->qmp_fd,1 );if (ret)return -1 ; +if (sizeof (qmp->addr.sun_path) <=strlen (qmp_socket_path)) +return -1 ; +memset (&qmp->addr,0 ,sizeof (qmp->addr)); qmp->addr.sun_family =AF_UNIX ;strncpy (qmp->addr.sun_path, qmp_socket_path, -sizeof (qmp->addr.sun_path)); +sizeof (qmp->addr.sun_path)-1 ); do { ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
Signature · 1 line
1.7.10.4
Quoted 16 lines · PRAMOD DEVENDRA
From: Pramod Devendra <[email protected]>
Signed-off-by Pramod Devendra <[email protected]>CC Ian Jackson <[email protected]>CC Stefano Stabellini <[email protected]>CC Ian Campbell <[email protected]>CC Wei Liu <[email protected]>--- tools/libxl/libxl_qmp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)tools/libxl/libxl_qmp.c +0
-0
tools/libxl/libxl_qmp.c static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path, · line 374
Quoted 6 lines
tools/libxl/libxl_qmp.c +3
-0@@ -369,10 +369,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path, ret =libxl_fd_set_cloexec (qmp->ctx, qmp->qmp_fd,1 );if (ret)return -1 ; +if (sizeof (qmp->addr.sun_path) <=strlen (qmp_socket_path)) +return -1 ; +
I know this is not your fault, but the function seems to leak qmp_fd on error path (qmp_fd is not closed). Do you fancy fixing that?
Wei.
Quoted 10 lines
tools/libxl/libxl_qmp.c +1
-1memset (&qmp->addr,0 ,sizeof (qmp->addr)); qmp->addr.sun_family =AF_UNIX ;strncpy (qmp->addr.sun_path, qmp_socket_path, -sizeof (qmp->addr.sun_path)); +sizeof (qmp->addr.sun_path)-1 ); do { ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,Signature · 1 line
1.7.10.4
On Mon, 2015-03-16 at 10:05 +0000, PRAMOD DEVENDRA wrote:
Please can you arrange in the future to chain your series into a single thread. That means that each N/3 mail should have a References and/or In-reply-to header pointing at either the 0/N mail or the (N-1)/N mail in the series.
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Sending_the_patches_to_the_list has some information on how to achieve this with git send-email.
Note that this also implies that you should include a 0/N mail which ties the series together (i.e. explains the overall purpose of the series). The wiki covers this too.
At the moment my mailbox contains 4 unthread mails from you, named 1/3, 2/3, 3/3 and 2/2 which is liable to get confusing if there are any further revisions etc.
Ian.
PATCH v2 1/1 libxl/libxl_qmp.c: fix qmp_open
In reply to PRAMOD DEVENDRAFrom: Pramod Devendra <[email protected]>
---
Changed since v1:
1. Make sure sun_path does not overflow.
2. Close qmp_fd on error.
--- tools/libxl/libxl_qmp.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
tools/libxl/libxl_qmp.c +20 -6
@@ -357,22 +357,32 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid) static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path, int timeout) { - int ret; + int ret = -1; int i = 0; qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0); if (qmp->qmp_fd < 0) { - return -1; + goto out; } ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1); - if (ret) return -1; + if (ret) { + ret = -1; + goto out; + } ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1); - if (ret) return -1; + if (ret) { + ret = -1; + goto out; + } + if (sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) { + ret = -1; + goto out; + } memset(&qmp->addr, 0, sizeof (qmp->addr)); qmp->addr.sun_family = AF_UNIX; strncpy(qmp->addr.sun_path, qmp_socket_path, - sizeof (qmp->addr.sun_path)); + sizeof (qmp->addr.sun_path)-1); do { ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
@@ -384,9 +394,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,* ECONNREFUSED : Leftover socket hasn't been removed yet */ continue ; } -return -1 ; + ret =-1 ; + goto out; }while ((++i /5 <= timeout) && (usleep (200 *1000 ) <=0 )); +out: +if (ret ==-1 && qmp->qmp_fd >-1 )close (qmp->qmp_fd); +return ret; }
Signature · 1 line
1.7.10.4
Re: [PATCH v2] libxl/libxl_qmp.c: fix qmp_open
Quoted 11 lines · PRAMOD DEVENDRA
From: Pramod Devendra <[email protected]>
Signed-off-by Pramod Devendra <[email protected]>CC Ian Jackson <[email protected]>CC Stefano Stabellini <[email protected]>CC Ian Campbell <[email protected]>CC Wei Liu <[email protected]>---
Changed since v1:
1. Make suresun_pathdoes not overflow.
2. Closeqmp_fdon error.
These lines should be in the commit log itself. Putting them here makes them get discarded when committing.
You also need to write about your third change, which was requested by Ian. That is
3. Use "goto out" error handling idiom.
Wei.
Re: [PATCH v2] libxl/libxl_qmp.c: fix qmp_open
Quoted 13 lines · Wei Liu
Quoted 11 lines · PRAMOD DEVENDRA
From: Pramod Devendra <[email protected]>
Signed-off-by Pramod Devendra <[email protected]>CC Ian Jackson <[email protected]>CC Stefano Stabellini <[email protected]>CC Ian Campbell <[email protected]>CC Wei Liu <[email protected]>---
Changed since v1:
1. Make suresun_pathdoes not overflow.
2. Closeqmp_fdon error.
Quoted 7 lines
These lines should be in the commit log itself. Putting them here makes them get discarded when committing.
You also need to write about your third change, which was requested by Ian. That is
3. Use "goto out" error handling idiom.
Please also use a subject more descriptive than "Fix foo", describe the fix somehow, since this combines multiple fixes perhaps "fix error handling in qmp_open" and then describe the various fixes it in the main bit of the message (i.e. the stuff about the path overflow and closing the fd).
In general it is easier for everyone to split such changes into two, then you can give each a useful and distinct subject.
Quoted 2 lines
Wei.
PATCH v3 1/1 libxl/libxl_qmp.c: fix error handling in qmp_open
In reply to PRAMOD DEVENDRAFrom: Pramod Devendra <[email protected]>
1. Make sure sun_path does not overflow
2. Close qmp_fd on error
3. Use goto out for error handling
--- tools/libxl/libxl_qmp.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
tools/libxl/libxl_qmp.c +20 -6
@@ -357,22 +357,32 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid) static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path, int timeout) { - int ret; + int ret = -1; int i = 0; qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0); if (qmp->qmp_fd < 0) { - return -1; + goto out; } ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1); - if (ret) return -1; + if (ret) { + ret = -1; + goto out; + } ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1); - if (ret) return -1; + if (ret) { + ret = -1; + goto out; + } + if (sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) { + ret = -1; + goto out; + } memset(&qmp->addr, 0, sizeof (qmp->addr)); qmp->addr.sun_family = AF_UNIX; strncpy(qmp->addr.sun_path, qmp_socket_path, - sizeof (qmp->addr.sun_path)); + sizeof (qmp->addr.sun_path)-1); do { ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
@@ -384,9 +394,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,* ECONNREFUSED : Leftover socket hasn't been removed yet */ continue ; } -return -1 ; + ret =-1 ; + goto out; }while ((++i /5 <= timeout) && (usleep (200 *1000 ) <=0 )); +out: +if (ret ==-1 && qmp->qmp_fd >-1 )close (qmp->qmp_fd); +return ret; }
Signature · 1 line
1.7.10.4
Re: [PATCH v3] libxl/libxl_qmp.c: fix error handling in qmp_open
Quoted 11 lines · PRAMOD DEVENDRA
From: Pramod Devendra <[email protected]>
1. Make sure
sun_pathdoes not overflow
2. Closeqmp_fdon error
3. Use goto out for error handlingSigned-off-by Pramod Devendra <[email protected]>CC Ian Jackson <[email protected]>CC Stefano Stabellini <[email protected]>CC Ian Campbell <[email protected]>CC Wei Liu <[email protected]>
Ian Campbell
AppliedRe: [PATCH v3] libxl/libxl_qmp.c: fix error handling in qmp_open
Quoted 7 lines · Wei Liu
Quoted 6 lines · PRAMOD DEVENDRA
From: Pramod Devendra <[email protected]>
1. Make sure
sun_pathdoes not overflow
2. Closeqmp_fdon error
3. Use goto out for error handling
Quoted 7 lines
Quoted 5 lines
Signed-off-by Pramod Devendra <[email protected]>CC Ian Jackson <[email protected]>CC Stefano Stabellini <[email protected]>CC Ian Campbell <[email protected]>CC Wei Liu <[email protected]>Acked-by Wei Liu <[email protected]>
Applied, thanks.
Pramod, my queue folder contains several patches from you with a confusing variety of versions ("2/2", "1/3", "v2"), however I think they are all precursors of this one and so I have moved them to my done folder. If there was another patch in there then please resend.