Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use g_free for xfs/g_xfs in xfs_delete_xfs_fs #1487

Merged
merged 1 commit into from Jan 30, 2020

Conversation

@derekschrock
Copy link
Contributor

derekschrock commented Jan 25, 2020

It's possible for xfs (g_xfs) to be null due to fuse mounts failing.
If g_xfs is null chansrv will crash with sig. fault.

@derekschrock

This comment has been minimized.

Copy link
Contributor Author

derekschrock commented Jan 26, 2020

By @metalefty request @matt335672 for review.

@matt335672

This comment has been minimized.

Copy link
Contributor

matt335672 commented Jan 27, 2020

Hi @derekschrock

Thanks for the PR. I've had a look at the code, and you're quite right that entering xfs_delete_xfs_fs() will cause a segfault if called with a NULL parameter. It's down to bad coding on my part which I didn't catch in my unit testing.

That said, I think your PR needs a bit of work for two reasons:-

  • It's valid to call free(p) with p being NULL. This has been the case for a very long time. It's even in the rather dog-eared copy of the 2nd edition K&R I have at home on page 252 (printed 1988) that [free] does nothing if P is NULL. So for XRDP, free(p) behaves identically to g_free(p), and whether to use one or the other is a question of style rather than behaviour.
  • To get to your patched line 400, the path of execution has to pass through the two preceding lines. These both de-reference the NULL pointer before calling free(), and so either would cause a segfault.

Assuming the above make sense, and I'm not missing something obvious could you update and re-submit the PR?

@metalefty - this code is referenced in my cppcheck PR #1481 (which co-incidentally also addresses this). Since Derek's putting some work in now, I'll happily rework that PR when we're done with this one. The other cppcheck PRs are not affected.

Thanks both

@derekschrock derekschrock force-pushed the derekschrock:devel branch from 66820a0 to 72bece5 Jan 28, 2020
@metalefty

This comment has been minimized.

Copy link
Member

metalefty commented Jan 29, 2020

Thank you both.

@matt335672 can you review @derekschrock 's new patch?

@matt335672

This comment has been minimized.

Copy link
Contributor

matt335672 commented Jan 29, 2020

LGTM - simplest solution to known problem. Thanks @derekschrock.

@metalefty metalefty merged commit f460343 into neutrinolabs:devel Jan 30, 2020
2 checks passed
2 checks passed
FreeBSD Task Summary
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@odeneriksson

This comment has been minimized.

Copy link

odeneriksson commented on 72bece5 Jan 30, 2020

Fixes the segfault on CentOS 8 for me.

@metalefty

This comment has been minimized.

Copy link
Member

metalefty commented Feb 4, 2020

We need to make a release for this fix.

@matt335672

This comment has been minimized.

Copy link
Contributor

matt335672 commented Feb 5, 2020

Agreed, as it seems to be affecting other users. I can only apologise for not picking it up in my unit testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.