Skip to content

bpo-27580: Add support of null characters in csv#28808

Merged
serhiy-storchaka merged 2 commits intopython:mainfrom
serhiy-storchaka:csv-nul
Oct 9, 2021
Merged

bpo-27580: Add support of null characters in csv#28808
serhiy-storchaka merged 2 commits intopython:mainfrom
serhiy-storchaka:csv-nul

Conversation

@serhiy-storchaka
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka commented Oct 7, 2021 •
edited by bedevere-bot
Loading

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Oct 7, 2021
@serhiy-storchaka serhiy-storchaka changed the title bpo-27580: Add support of null characters in :mod:csv. bpo-27580: Add support of null characters in csv Oct 7, 2021
@serhiy-storchaka serhiy-storchaka requested review from encukou and removed request for encukou October 7, 2021 18:54
@serhiy-storchaka
Copy link
Copy Markdown
Member Author

serhiy-storchaka commented Oct 7, 2021

@smontanaro, please take a look. For unknown reasons I cannot add you to reviewers.

@smontanaro
Copy link
Copy Markdown
Contributor

smontanaro commented Oct 7, 2021

For unknown reasons I cannot add you to reviewers.

Maybe that's by design. :-)

LGTM. I assume Doc/library/csv.rst will be updated at some point to make it clear that ASCII NUL is now a valid.

@ammaraskar
Copy link
Copy Markdown
Member

ammaraskar commented Oct 7, 2021

You don't have to necessarily do it here, but just a note to myself that we should update the csv fuzzer:

cpython/Modules/_xxtestfuzz/fuzzer.c

Lines 356 to 360 in 0219017

/* Ignore non null-terminated strings since _csv can't handle
embedded nulls */
if (memchr(data, '\0', size) == NULL) {
return 0;
}

to generate inputs with nulls, such as in the json fuzzer here:

cpython/Modules/_xxtestfuzz/fuzzer.c

Lines 166 to 169 in 0219017

PyObject* input_bytes = PyBytes_FromStringAndSize(data, size);
if (input_bytes == NULL) {
return 0;
}

in this or a follow up PR.

corona10
corona10 approved these changes Oct 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 mentioned this pull request Oct 9, 2021
Merged
@serhiy-storchaka
Copy link
Copy Markdown
Member Author

serhiy-storchaka commented Oct 9, 2021

I assume Doc/library/csv.rst will be updated at some point to make it clear that ASCII NUL is now a valid.

I have not found a good place for this. It was not documented that ASCII NUL is special. Perhaps a NEWS entry will be enough.

@serhiy-storchaka
Copy link
Copy Markdown
Member Author

serhiy-storchaka commented Oct 9, 2021

@ammaraskar, I have no experience with fuzzers. Feel free to add a new test if it is useful.

@serhiy-storchaka serhiy-storchaka merged commit b454e8e into python:main Oct 9, 2021
@serhiy-storchaka serhiy-storchaka deleted the csv-nul branch October 9, 2021 16:17
@lithomas1 lithomas1 mentioned this pull request Aug 17, 2022
Merged
5 tasks
@merwok
Copy link
Copy Markdown
Member

merwok commented Nov 8, 2022

#97503 is requesting a backport to 3.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes

@corona10 corona10 corona10 approved these changes

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@serhiy-storchaka @smontanaro @ammaraskar @merwok @iritkatriel @corona10 @the-knights-who-say-ni @bedevere-bot