Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] fsdev/virtfs-proxy-helper: Fix possible overflow

[Stefan Hajnoczi]

On Mon, Mar 30, 2015 at 07:33:33PM +0530, Aneesh Kumar K.V wrote:
> Stefan Hajnoczi <address@hidden> writes:
> 
> > On Sat, Mar 14, 2015 at 10:00:16AM +0800, Shannon Zhao wrote:
> >> It's detected by coverity. As max of sockaddr_un.sun_path is
> >> sizeof(helper.sun_path), should check the length of source
> >> and use strncpy instead of strcpy.
> >> 
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> ---
> >>     v1->v2: Still use strcpy [Paolo]
> >> ---
> >>  fsdev/virtfs-proxy-helper.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> >> index bf2e5f3..13fe032 100644
> >> --- a/fsdev/virtfs-proxy-helper.c
> >> +++ b/fsdev/virtfs-proxy-helper.c
> >> @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, 
> >> gid_t gid)
> >>          return -1;
> >>      }
> >>  
> >> +    g_assert(strlen(path) < sizeof(proxy.sun_path));
> >>      sock = socket(AF_UNIX, SOCK_STREAM, 0);
> >
> > path is user input.  While the assertion check silences Coverity, it is
> > not suitable for input validation.  Users expect a graceful exit with an
> > error message, not an assertion failure if the given path is too long.
> >
> > I will send a patch.
> 
> That is the proxy helper. The assert will cause an exit() which is good,
> isn't it ? I did update the qemu side of the patch to do a graceful exit

assert(3) calls abort(3), which sends SIGABRT.  The signal causes a core
dump by default.  That's not a proper way to exit on invalid input.

Plus, if the code is ever compiled with the NDEBUG macro set, the assert
would be removed and the check bypassed.

This is why assert() isn't suitable for input validation.

Stefan

pgpIIdPtsq6oh.pgp
Description: PGP signature