Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

Fixes exception when passing incorrect bytes to decode#190

Merged
ncordon merged 3 commits intobblfsh:masterfrom
ncordon:fix.empty.decode
Sep 16, 2019
Merged

Fixes exception when passing incorrect bytes to decode#190
ncordon merged 3 commits intobblfsh:masterfrom
ncordon:fix.empty.decode

Conversation

@ncordon
Copy link
Member

@ncordon ncordon commented Sep 11, 2019

An uncaught exception was thrown and python interpreter was killed because of that.

Closes #189

Signed-off-by: ncordon nacho.cordon.castillo@gmail.com


This change is Reviewable

An uncaught exception was thrown and python interpreter was killed because of that

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@ncordon ncordon requested a review from creachadair September 11, 2019 17:06
Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ncordon)


bblfsh/pyuast.cc, line 1060 at r1 (raw file):

      if (!pyU) {
        delete(ctx);
        return nullptr;

In this case we haven't released buf. Should we fall through as you did in the case below?

@ncordon
Copy link
Member Author

ncordon commented Sep 11, 2019


bblfsh/pyuast.cc, line 1060 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

In this case we haven't released buf. Should we fall through as you did in the case below?

I think buf would be released after the try-catch, or am I missing something?

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ncordon)


bblfsh/pyuast.cc, line 1060 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

I think buf would be released after the try-catch, or am I missing something?

I don't see how—if we return nullptr at this point control exits the function and that code never runs, right?

@ncordon
Copy link
Member Author

ncordon commented Sep 11, 2019


bblfsh/pyuast.cc, line 1060 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

I don't see how—if we return nullptr at this point control exits the function and that code never runs, right?

Oh yes I missed that, my bad. Fixing it

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

That early return could provoke the buffer not being released

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


bblfsh/pyuast.cc, line 5 at r2 (raw file):

#include <cstring>
#include <unordered_map>
#include <iostream>

I was also wondering why it's there, but forgot to ask :)

@ncordon
Copy link
Member Author

ncordon commented Sep 13, 2019


bblfsh/pyuast.cc, line 5 at r2 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I was also wondering why it's there, but forgot to ask :)

I erased it

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ncordon ncordon merged commit dd528f9 into bblfsh:master Sep 16, 2019
@ncordon ncordon deleted the fix.empty.decode branch September 16, 2019 16:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect bytes in decode kill the python interpreter

3 participants