Check for null byte at the time of writing a file.

[damien-biasotto]

Good afternoon everyone,

I decided to tackle the #959 issue, by adding a byte null check in the protected method _IsValidSource.

However, I introduced a new behaviour, I arbitrarily decided to flag any strings that contain a null byte character as invalid (for safety reasons).

Then removed any null byte related error suppressors.

I'm not sure what are the guidelines for Pull Requests. Shall I provide a unit test to highlight this change?

[tmotyl]

what else can cause a warning except for the null byte?

[damien-biasotto]

what else can cause a warning except for the null byte?

Well, the documentation is not really exhaustive :

Upon failure, an E_WARNING is emitted.

I'd assume if you're not passing a string nor a resource?

[tmotyl]

hmm seems this change introduced an issue: #1084
How can it be possible that the problem was not caught by linter?

[sreichel]

@tmotyl this is no problem with linter, it's a problem with reviews ... :(

I've checked https://www.php.net/manual/en/security.filesystem.nullbytes.php, but did not see the typo in that PR AND it was not really tested. Varien_Io_File::write() is only used in a few places so this does not popup during normal frontend/backend tests.

A clear description, steps to reproduce and maybe a code snippet (if easier) should be mandatory.

[damien-biasotto]

Oh my, I'm really sorry guys. I don't know how i didn't catch that.

[Flyingmana]

@tmotyl I added an Issue for this, I also think we should have a static analyser, which can find such a case.
#1087

The current one did not catch it, as we just check for syntax error, and technically this is not a syntax error.

[digitalpianism]

I made a PR to address the bug that was introduced by that change: #1302

[Tomasz-Silpion]

After hours of debug why our image import has stopped to work what was spotted more than 2 weeks from OpenMage update I got into the same Varien_Io_File _IsValidSource method $src, chr(0) check that fails on base64 content. Replacing the class with @digitalpianism patch has solved the problem. @damien-biasotto @tmotyl can you review the change and merge it for the next release?