Skip to content

BUG: Series.unique segfaults on invalid unicode #34550

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

Closed
marco-neumann-by opened this issue Jun 3, 2020 · 6 comments · Fixed by #34851
Closed

BUG: Series.unique segfaults on invalid unicode #34550

marco-neumann-by opened this issue Jun 3, 2020 · 6 comments · Fixed by #34851
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff good first issue Needs Tests Unit test(s) needed to prevent regressions
Milestone

Comments

@marco-neumann-by
Copy link

marco-neumann-by commented Jun 3, 2020

Not present anymore, might be fixed by accident, but no could not find a PR that did that.

Code Sample, a copy-pastable example

import pandas as pd

ser = pd.Series(["\ud83d"])
ser.unique()

Problem description

UnicodeEncodeError: 'utf-8' codec can't encode character '\ud83d' in position 0: surrogates not allowed
Exception ignored in: 'pandas._libs.tslibs.util.get_c_string_buf_and_size'                                             
UnicodeEncodeError: 'utf-8' codec can't encode character '\ud83d' in position 0: surrogates not allowed
Segmentation fault (core dumped)             

Expected Output

Not crashing.

Output of pd.show_versions()

INSTALLED VERSIONS

commit : None
python : 3.6.6.final.0
python-bits : 64
OS : Linux
OS-release : 5.4.0-33-generic
machine : x86_64
processor :
byteorder : little
LC_ALL : en_US.UTF-8
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.0.4
numpy : 1.18.1
pytz : 2019.3
dateutil : 2.8.1
pip : 19.2.3
setuptools : 41.2.0
Cython : None
pytest : 5.4.1
hypothesis : None
sphinx : 3.0.3
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : 2.11.2
IPython : 7.15.0
pandas_datareader: None
bs4 : None
bottleneck : None
fastparquet : None
gcsfs : None
lxml.etree : None
matplotlib : None
numexpr : None
odfpy : None
openpyxl : None
pandas_gbq : None
pyarrow : 0.15.1
pytables : None
pytest : 5.4.1
pyxlsb : None
s3fs : None
scipy : None
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
xlwt : None
xlsxwriter : None
numba : None

@marco-neumann-by marco-neumann-by added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 3, 2020
@phofl
Copy link
Member

phofl commented Jun 3, 2020

Hey,

thanks for your report. This works for me on master. Output:

['\ud83d']

Which commit did you test on? I pulled before executing this.

@jorisvandenbossche jorisvandenbossche added Regression Functionality that used to work in a prior pandas version and removed Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 4, 2020
@jorisvandenbossche jorisvandenbossche added this to the 1.0.5 milestone Jun 4, 2020
@jorisvandenbossche
Copy link
Member

This is indeed fixed on master. I suppose by #32701 and #33235

@marco-neumann-by
Copy link
Author

Oh sorry, it seems I wasn't clear enough ("Not present anymore, might be fixed by accident, but no could not find a PR that did that."). I've meant that it currently is fixed on master, but I wasn't sure if it was fixed by some PR just "by accident" (i.e. some refactoring / bugfix that was meant to address something else) or if it is even present on master (SEGFAULTs can be somewhat tricky due to their dependency on memory layout and certain runtime aspects).

@jorisvandenbossche
Copy link
Member

Ah, I read over your note (BTW, feel free to edit the top of the issue template, as I did now, it's mostly to remind people to do that when they are writing the issue, but you don't necessarily need to keep it once written. That can make your issue easier to read)

But indeed, I suppose it was kind of by accident (it was the same unicode string, but causing an error in another context, not a segfault), so at least we want to add tests for this case.

@arw2019
Copy link
Member

arw2019 commented Jun 5, 2020

@jorisvandenbossche @marco-neumann-jdas @phofl I'm happy to do a PR with these tests if nobody is working on this already!

@jbrockmendel jbrockmendel added the Segfault Non-Recoverable Error label Jun 5, 2020
@TomAugspurger TomAugspurger added Needs Tests Unit test(s) needed to prevent regressions good first issue and removed Regression Functionality that used to work in a prior pandas version Segfault Non-Recoverable Error labels Jun 12, 2020
@TomAugspurger TomAugspurger modified the milestones: 1.0.5, Contributions Welcome Jun 12, 2020
@suvayu
Copy link
Contributor

suvayu commented Jun 16, 2020

I would like to work on this issue as part of the PyData Amsterdam coding sprint.

So far I have confirmed that the released version of Pandas has the bug, and master is fixed. AFAIU, it needs a regression test. The issue was, for the specific payload of invalid unicode data calling pandas.Series.unique(..) was triggering a segfault. To cover similar issues in the future, I would imagine the test shouldn't limit itself to the case of calling Series.unique(..).

So it should be added in the same place where Series is being tested for different kinds of payloads, is my thinking correct here? If so, I'm unclear where I should add the test. I see base/test_unique.py has tests for Series.unique, and arrays/string_/test_string.py has tests for string payloads. Am I looking at the right place? Can someone please guide me? Also, how can I mark the test I add as a regression test?

@jreback jreback modified the milestones: Contributions Welcome, 1.1 Jun 18, 2020
@jreback jreback added the Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff label Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff good first issue Needs Tests Unit test(s) needed to prevent regressions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants