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

readline: don't dereference possible NULL pointer #5065

Merged
merged 1 commit into from Jan 28, 2019

Conversation

Projects
None yet
4 participants
@jtgrassie
Copy link
Contributor

commented Jan 12, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

That seems racy since rl_prompt seems to be changing elsewhere async.
At the least you should copy to a local first.
Reading the code, maybe it's the calls to rl_set_prompt, which aren't getting the sync_mutex.

@hyc

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

There shouldn't ever be more than 1 thread trying to read keyboard input. I don't see how there can be any races with set_prompt.

@jtgrassie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

@moneromooo-monero I looked at the readline source and there are a few ways rl_prompt can be NULL which is why this check is needed. It's not a race issue as far as I can tell. I did consider putting a sync lock on the install/remove_line_handler functions but this check should suffice.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Do you mean the sync call here, which is called by libstdc++, cannot happen at the same time as that particular readline code runs ?

@jtgrassie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

No I meant a lock on our sync_mutex. Because the install/remove_line_handler functions call through to readline code that can temporarily free rl_prompt. But as I say, shouldn't be needed with our ref check now.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

From my reading of the code, admittedly quick, start/stop can be called by the PAUSE_READLINE function, which is whenever the user inputs something. The sync call is called (at least) when a certain network event happens. It is not clear to me why those do not race.

@jtgrassie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

When readline buffer is paused, sync cannot be called though on it's stream.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Why can it not ?

@jtgrassie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

Because cout's buffer is switched back to it's original stream upon stop of readline buffer. Thus there is nothing that can flush the readline buffer causing a sync.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

Ah, I see. This is the rdbuf call, right ? If so, then if does look racy to me since those calls are not atomic with the install/remove of the handler.

@jtgrassie

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

This is the rdbuf call, right ?

Right.

those calls are not atomic with the install/remove of the handler.

But that doesn't (shouldn't) matter in this context. Hence my comment:

I did consider putting a sync lock on the install/remove_line_handler functions but this check should suffice.

The race is that rl_prompt can be NULL for a moment, but in our stream buffer sync, if it is, so long as we check before use, it should be fine. We can lock a mutex but I feel it's unnecessary overhead and may even cause undesirable consequences.

@moneromooo-monero
Copy link
Contributor

left a comment

It's definitely better now, even though I'm not convinced there's no race :)

@jtgrassie jtgrassie force-pushed the jtgrassie:readline-deref branch from b3b3ac2 to ca86ef1 Jan 21, 2019

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit ca86ef1 into monero-project:master Jan 28, 2019

6 of 10 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details

fluffypony added a commit that referenced this pull request Jan 28, 2019

Merge pull request #5065
ca86ef1 readline: don't dereference possible NULL pointer (Jethro Grassie)

@jtgrassie jtgrassie deleted the jtgrassie:readline-deref branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.