Re: [Nfs-ganesha-devel] address sanitizer defeats -O0

2017-08-03 Thread Matt Benjamin 
Correct.

Matt

On Thu, Aug 3, 2017 at 10:44 AM, Daniel Gryniewicz  wrote:
> I do not believe it's reorganizing code.   I think the code in the first
> section is wrong, and the second version is needed.
>
> Once you've dec'd the refcount, you cannot access the pointer.  At that
> point, another thread could deref and free, and you'd be left with freed
> memory.  You *must* store that value in a local variable.
>
> Daniel
>
>
> On 08/03/2017 02:13 AM, William Allen Simpson wrote:
>>
>> It's improperly reorganizing code.
>>
>> ===
>>
>> Accessing freed memory at ==>
>>
>> Logically, this couldn't happen!
>>
>> int free_nfs_request(request_data_t *reqdata)
>> {
>>  atomic_dec_uint32_t(&reqdata->r_d_refs);
>>
>>  LogDebug(COMPONENT_DISPATCH,
>>   "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
>>   __func__,
>>   reqdata->r_u.req.svc.rq_xprt,
>>   reqdata->r_u.req.svc.rq_xprt->xp_fd,
>>   reqdata->r_u.req.svc.rq_xprt->xp_refs,
>>   reqdata->r_d_refs);
>>
>>  if (reqdata->r_d_refs)
>> ==> return reqdata->r_d_refs;
>>
>>  switch (reqdata->rtype) {
>>  case NFS_REQUEST:
>>  /* dispose RPC header */
>>  if (reqdata->r_u.req.svc.rq_auth)
>>  SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
>>  XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
>>  break;
>>  default:
>>  break;
>>  }
>>  SVC_RELEASE(reqdata->r_u.req.svc.rq_xprt, SVC_RELEASE_FLAG_NONE);
>>  pool_free(request_pool, reqdata);
>>  return 0;
>> }
>>
>> ===
>>
>> This works by using local variables.  Kinda pointlessly, when not
>> running log debug.
>>
>> int free_nfs_request(request_data_t *reqdata)
>> {
>>  SVCXPRT *xprt = reqdata->r_u.req.svc.rq_xprt;
>>  uint32_t refs = atomic_dec_uint32_t(&reqdata->r_d_refs);
>>
>>  LogDebug(COMPONENT_DISPATCH,
>>   "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
>>   __func__,
>>   xprt, xprt->xp_fd, xprt->xp_refs,
>>   refs);
>>
>>  if (refs)
>>  return refs;
>>
>>  switch (reqdata->rtype) {
>>  case NFS_REQUEST:
>>  /* dispose RPC header */
>>  if (reqdata->r_u.req.svc.rq_auth)
>>  SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
>>  XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
>>  break;
>>  default:
>>  break;
>>  }
>>  SVC_RELEASE(xprt, SVC_RELEASE_FLAG_NONE);
>>  pool_free(request_pool, reqdata);
>>  return 0;
>> }
>>
>>
>> --
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> ___
>> Nfs-ganesha-devel mailing list
>> Nfs-ganesha-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
>
>
>
> --
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> ___
> Nfs-ganesha-devel mailing list
> Nfs-ganesha-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

Re: [Nfs-ganesha-devel] address sanitizer defeats -O0

2017-08-03 Thread Daniel Gryniewicz 
I do not believe it's reorganizing code.   I think the code in the first 
section is wrong, and the second version is needed.


Once you've dec'd the refcount, you cannot access the pointer.  At that 
point, another thread could deref and free, and you'd be left with freed 
memory.  You *must* store that value in a local variable.


Daniel

On 08/03/2017 02:13 AM, William Allen Simpson wrote:

It's improperly reorganizing code.

===

Accessing freed memory at ==>

Logically, this couldn't happen!

int free_nfs_request(request_data_t *reqdata)
{
 atomic_dec_uint32_t(&reqdata->r_d_refs);

 LogDebug(COMPONENT_DISPATCH,
  "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
  __func__,
  reqdata->r_u.req.svc.rq_xprt,
  reqdata->r_u.req.svc.rq_xprt->xp_fd,
  reqdata->r_u.req.svc.rq_xprt->xp_refs,
  reqdata->r_d_refs);

 if (reqdata->r_d_refs)
==> return reqdata->r_d_refs;

 switch (reqdata->rtype) {
 case NFS_REQUEST:
 /* dispose RPC header */
 if (reqdata->r_u.req.svc.rq_auth)
 SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
 XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
 break;
 default:
 break;
 }
 SVC_RELEASE(reqdata->r_u.req.svc.rq_xprt, SVC_RELEASE_FLAG_NONE);
 pool_free(request_pool, reqdata);
 return 0;
}

===

This works by using local variables.  Kinda pointlessly, when not
running log debug.

int free_nfs_request(request_data_t *reqdata)
{
 SVCXPRT *xprt = reqdata->r_u.req.svc.rq_xprt;
 uint32_t refs = atomic_dec_uint32_t(&reqdata->r_d_refs);

 LogDebug(COMPONENT_DISPATCH,
  "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
  __func__,
  xprt, xprt->xp_fd, xprt->xp_refs,
  refs);

 if (refs)
 return refs;

 switch (reqdata->rtype) {
 case NFS_REQUEST:
 /* dispose RPC header */
 if (reqdata->r_u.req.svc.rq_auth)
 SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
 XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
 break;
 default:
 break;
 }
 SVC_RELEASE(xprt, SVC_RELEASE_FLAG_NONE);
 pool_free(request_pool, reqdata);
 return 0;
}

-- 


Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel



--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel

[Nfs-ganesha-devel] address sanitizer defeats -O0

2017-08-03 Thread William Allen Simpson 

It's improperly reorganizing code.

===

Accessing freed memory at ==>

Logically, this couldn't happen!

int free_nfs_request(request_data_t *reqdata)
{
atomic_dec_uint32_t(&reqdata->r_d_refs);

LogDebug(COMPONENT_DISPATCH,
 "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
 __func__,
 reqdata->r_u.req.svc.rq_xprt,
 reqdata->r_u.req.svc.rq_xprt->xp_fd,
 reqdata->r_u.req.svc.rq_xprt->xp_refs,
 reqdata->r_d_refs);

if (reqdata->r_d_refs)
==> return reqdata->r_d_refs;

switch (reqdata->rtype) {
case NFS_REQUEST:
/* dispose RPC header */
if (reqdata->r_u.req.svc.rq_auth)
SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
break;
default:
break;
}
SVC_RELEASE(reqdata->r_u.req.svc.rq_xprt, SVC_RELEASE_FLAG_NONE);
pool_free(request_pool, reqdata);
return 0;
}

===

This works by using local variables.  Kinda pointlessly, when not
running log debug.

int free_nfs_request(request_data_t *reqdata)
{
SVCXPRT *xprt = reqdata->r_u.req.svc.rq_xprt;
uint32_t refs = atomic_dec_uint32_t(&reqdata->r_d_refs);

LogDebug(COMPONENT_DISPATCH,
 "%s: %p fd %d xp_refs %" PRIu32 " r_d_refs %" PRIu32,
 __func__,
 xprt, xprt->xp_fd, xprt->xp_refs,
 refs);

if (refs)
return refs;

switch (reqdata->rtype) {
case NFS_REQUEST:
/* dispose RPC header */
if (reqdata->r_u.req.svc.rq_auth)
SVCAUTH_RELEASE(&(reqdata->r_u.req.svc));
XDR_DESTROY(reqdata->r_u.req.svc.rq_xdrs);
break;
default:
break;
}
SVC_RELEASE(xprt, SVC_RELEASE_FLAG_NONE);
pool_free(request_pool, reqdata);
return 0;
}

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Nfs-ganesha-devel mailing list
Nfs-ganesha-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel