Issue #220: Extra data in section field causing Unicode issues and #221 pls_recall

Issue 220: Extra data in section field causing Unicode issues.

[kiddi, 2015-06-02 23:15:02]

Code updated.

[Joachim Metz, 2015-06-03 05:30:00]

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py
File plaso/parsers/pe.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:130: section_name_full = getattr(section, u'Name', b'')
We'll need to discuss this with Daniel.
His idea was the format "string\x90"

I'm also wondering why you see "string\x00\x90" looks to me the "\x90" is just
remnant info.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall.py
File plaso/parsers/pls_recall.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:62: # 6 * 365 * 24 * 60 * 60 * 1e6
1e6 => 1000000

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:141: # Verify that the timestamp is no more than six
years into the future.
Explain why 6 years

[onager, 2015-06-03 11:10:26]

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py
File plaso/parsers/pe.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:132: section_name, _, _ =
section_name_full.partition(b'\x00')
I don't think this correct - the section name is always 8 bytes, it isn't NUL
terminated, but NUL padded:

From the MS documentation at
http://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8...

"An 8-byte, null-padded UTF-8 encoded string. If the string is exactly
8 characters long, there is no terminating null."

u'string\x00\x90' is the desired string here, IMO, not u'string' or
u'string\x90'.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:135: except UnicodeDecodeError:
Is this possible? I don't see how trying to encode with unicode_escape can ever
trigger an exception - have you seen something?

[onager, 2015-06-03 11:15:13]

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py
File plaso/parsers/pe.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:135: except UnicodeDecodeError:
On 2015/06/03 11:10:25, onager wrote:
> Is this possible? I don't see how trying to encode with unicode_escape can
ever
> trigger an exception - have you seen something?

Duh, didn't look at the issue. Ignore me

[onager, 2015-06-03 11:30:41]

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py
File plaso/parsers/pe.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:134: section_name =
u'{0:s}'.format(section_name.encode(u'unicode_escape'))
Duh, this should be:

section_name = codecs.decode(section_name, u'unicode_escape')

To get the desired behaviour.

Encode is just wrong, and I don't know how this "fix" I did ever worked.

[kiddi, 2015-06-03 19:54:37]

done, updated.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py
File plaso/parsers/pe.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:130: section_name_full = getattr(section, u'Name', b'')
On 2015/06/03 05:30:00, Joachim Metz wrote:
> We'll need to discuss this with Daniel.
> His idea was the format "string\x90"
> 
> I'm also wondering why you see "string\x00\x90" looks to me the "\x90" is just
> remnant info.

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:132: section_name, _, _ =
section_name_full.partition(b'\x00')
On 2015/06/03 11:10:25, onager wrote:
> I don't think this correct - the section name is always 8 bytes, it isn't NUL
> terminated, but NUL padded:
> 
> From the MS documentation at
>
http://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8...
> 
> "An 8-byte, null-padded UTF-8 encoded string. If the string is exactly
> 8 characters long, there is no terminating null."
> 
> u'string\x00\x90' is the desired string here, IMO, not u'string' or
> u'string\x90'.
> 
> 

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:134: section_name =
u'{0:s}'.format(section_name.encode(u'unicode_escape'))
On 2015/06/03 11:30:41, onager wrote:
> Duh, this should be:
> 
> section_name = codecs.decode(section_name, u'unicode_escape')
> 
> To get the desired behaviour.
> 
> Encode is just wrong, and I don't know how this "fix" I did ever worked.

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:135: except UnicodeDecodeError:
On 2015/06/03 11:15:13, onager wrote:
> On 2015/06/03 11:10:25, onager wrote:
> > Is this possible? I don't see how trying to encode with unicode_escape can
> ever
> > trigger an exception - have you seen something?
> 
> Duh, didn't look at the issue. Ignore me

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pe.py#newco...
plaso/parsers/pe.py:135: except UnicodeDecodeError:
On 2015/06/03 11:10:25, onager wrote:
> Is this possible? I don't see how trying to encode with unicode_escape can
ever
> trigger an exception - have you seen something?

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall.py
File plaso/parsers/pls_recall.py (right):

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:62: # 6 * 365 * 24 * 60 * 60 * 1e6
On 2015/06/03 05:30:00, Joachim Metz wrote:
> 1e6 => 1000000

Done.

https://codereview.appspot.com/245760043/diff/20001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:141: # Verify that the timestamp is no more than six
years into the future.
On 2015/06/03 05:30:00, Joachim Metz wrote:
> Explain why 6 years

well, that is just some arbitrary number.... just to check for some value of
"correctness", it might as well be 1 week, or 5 years, 2 months... added
something to the doc string

[kiddi, 2015-06-03 19:55:38]

Code updated.

[Joachim Metz, 2015-06-03 20:40:24]

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall.py
File plaso/parsers/pls_recall.py (right):

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:142: # Six years is an arbitrary timelength just to
evaluate the timestamp
timelength => time length ?

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:144: # TODO: Add a check for similarly valid value
back in time. Maybe if it the
I opt to add a TODO that this is a very flaky assumption.

[Joachim Metz, 2015-06-03 20:41:38]

LGTM, if you fix the remaining nits

[kiddi, 2015-06-03 20:57:19]

done

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall.py
File plaso/parsers/pls_recall.py (right):

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:142: # Six years is an arbitrary timelength just to
evaluate the timestamp
On 2015/06/03 20:40:24, Joachim Metz wrote:
> timelength => time length ?

Done.

https://codereview.appspot.com/245760043/diff/40001/plaso/parsers/pls_recall....
plaso/parsers/pls_recall.py:144: # TODO: Add a check for similarly valid value
back in time. Maybe if it the
On 2015/06/03 20:40:24, Joachim Metz wrote:
> I opt to add a TODO that this is a very flaky assumption.

Done.

[kiddi, 2015-06-03 20:58:29]

Code Submitted.