search

sahlberg / libnfs

NFS client library
Other
510 stars 200 forks source link

Re: rpc_set_error: set error to "out of memory" if the malloc failed #465

Closed linuxsmiths closed 4 months ago

linuxsmiths commented 4 months ago

@sahlberg, I had sent out a PR that conflicts with the above commit. I will not update it now but you can make the following changes which are currently lacking:

  1. rpc_destroy_context() also needs to have the awareness of "oom" pointer being special and that should not be de-allocated.
  2. We need to release the lock in case we fail with oom error.
  3. nfs_set_error{,_locked} must also behave the same for consistency.
sahlberg commented 4 months ago

No worries. I will do that tomorrow morning.

On Thu, 2 May 2024 at 18:46, linuxsmiths @.***> wrote:

@sahlberg https://github.com/sahlberg, I had sent out a PR that conflicts with the above commit. I will not update it now but you can make the following changes which are currently lacking:

  1. rpc_destroy_context() also needs to have the awareness of "oom" pointer being special and that should not be de-allocated.
  2. We need to release the lock in case we fail with oom error.
  3. nfs_set_error{,_locked} must also behave the same for consistency.

— Reply to this email directly, view it on GitHub https://github.com/sahlberg/libnfs/issues/465, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADY3EHXLNRZMWE4EIVDQZDZAH4P7AVCNFSM6AAAAABHDJFZCWVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI3TIOJUG43DKMQ . You are receiving this because you were mentioned.Message ID: @.***>

sahlberg commented 4 months ago

done

linuxsmiths commented 4 months ago

done

thanks! I'm looking on my mobile device but I don't seem to see the equivalent change in nfs_destroy_context()?

Also, current code may cause double free of old_error_string() in both rpc_set_error() and nfs_set_error() in case of oom. Sorry, I cannot send a PR myself, but pls do look at my aborted PR which was taking care of the above and was also logging in case of oom which is desirable. Thanks.

sahlberg commented 4 months ago

You are right. thanks.