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

Check before attempting to perform a zero-length fread #72

Merged

Conversation

matcho
Copy link

@matcho matcho commented Dec 9, 2015

Avoids a Warning thrown by fread() in case of zero-length message body

@@ -510,8 +510,12 @@ private function getPartBodyFromFile(&$part)
{
$start = $part['starting-pos-body'];
$end = $part['ending-pos-body'];
fseek($this->stream, $start, SEEK_SET);
return fread($this->stream, $end-$start);
$body = '';

Choose a reason for hiding this comment

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

Spaces must be used to indent lines; tabs are not allowed

@eXorus
Copy link
Member

eXorus commented Dec 10, 2015

Thanks for your contribution to improve the quality.

Do you have a test file where start > end to add it to the project ?

Also I prefer to write

if ($end-$start>0) return '';

fseek($this->stream, $start, SEEK_SET);
return fread($this->stream, $end-$start);

What do you think ?

@@ -510,6 +510,7 @@ private function getPartBodyFromFile(&$part)
{
$start = $part['starting-pos-body'];
$end = $part['ending-pos-body'];
if ($end-$start <= 0) return '';

Choose a reason for hiding this comment

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

Inline control structures are not allowed

@matcho
Copy link
Author

matcho commented Dec 10, 2015

I prefer your way too, just didn't want to be shout at because of introducing multiple returns, that tend to irritate a lot of people ^^
Pull request updated.
Attached, an email (disguised headers) with a zero-length body content, that comes from a mailing list. Until now I've never been confronted to a body length < 0.

Salut depuis Montpellier !
zero-length-body-email-example.txt

@matcho
Copy link
Author

matcho commented Dec 10, 2015

lol, the auto reviewer shouts at us now ^^

@@ -510,6 +510,9 @@ private function getPartBodyFromFile(&$part)
{
$start = $part['starting-pos-body'];
$end = $part['ending-pos-body'];
if ($end-$start <= 0) {
return '';

Choose a reason for hiding this comment

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

Spaces must be used to indent lines; tabs are not allowed

@matcho
Copy link
Author

matcho commented Dec 10, 2015

Mhh... and now coverage decreases because of multiple returns, I guess. What do you think ? Do I revert back to the first version ? Ultimately, it seems to be the most politically correct way, regarding coverage requirements.
Edit : ugh, and spaces again - I must be more careful !

@eXorus
Copy link
Member

eXorus commented Dec 10, 2015

No we need to add the test file to the test and check that body return '' in the case of zero-length body content

added a test with a zero-length body example message
@matcho
Copy link
Author

matcho commented Dec 10, 2015

Mh, I don't understand why Travis CI build failed (I'm pretty new to all those tools). Anyway all the tests work on my copy, so my skills stop here, I lend this over to you.

Cheers

@eXorus
Copy link
Member

eXorus commented Dec 10, 2015

Thanks I will try to merge soon.

@CSG7
Copy link

CSG7 commented Feb 7, 2016

Sorry for putting pressure to this, but I think this is a critical error which should be merged soon.

eXorus added a commit that referenced this pull request Feb 9, 2016
Check before attempting to perform a zero-length fread
@eXorus eXorus merged commit 22c8151 into php-mime-mail-parser:master Feb 9, 2016
@eXorus
Copy link
Member

eXorus commented Feb 9, 2016

Thanks to pressure me, I totaly forgot it.
Now it's published in 2.2.1

@matcho matcho deleted the fix/zero-length-fread branch March 9, 2016 17:00
@coveralls
Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.006%) to 99.628% when pulling ed11963 on telabotanica:fix/zero-length-fread into 92b5fea on php-mime-mail-parser:master.

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

Successfully merging this pull request may close these issues.

None yet

5 participants