NaCl: Fix NaClBrowserTestPnaclNonSfi.IrtManifestFile

NaCl: Fix NaClBrowserTestPnaclNonSfi.IrtManifestFile

We have switched from fgets to read, but the change did not
modify code which converts CRLF to LF.

https://codereview.chromium.org/133033002

Due to this reason, we were running strlen for the buffer
filled by read, which is not always NULL terminated.

This change simply removes the CRLF-to-LF conversion. This is
safe because we do not have any newlines in the test file.

BUG=392768
TEST=./out/Debug/browser_tests --gtest_filter='*NonSfi*IrtMani*'
TEST=trybots

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282838

[Mark Seaborn, 2014-07-11 16:55:19]

So the reason got disabled when switching to Clang wasn't anything to do
with Clang?  Switching to Clang (for trusted code) shouldn't even affect
the untrusted code that was buggy here.

> As we do not use FILE* anymore, Windows specific CRLF issues should
> not happen.

That's not quite true.  Neither fgets() nor read() will perform Windows
CRLF conversion when running as NaCl untrusted code, which assumes Unix
conventions.

The problem described in the comment could still happen when modifying
the test file on the trybots or locally on Windows, but only if the file
actually contains newlines.  The file clearly doesn't for
irt_manifest_file_test.cc.  I'm not sure about the other tests, but
assuming they don't use newlines, then LGTM.

https://codereview.chromium.org/383063002/diff/1/chrome/test/data/nacl/manife...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (left):

https://codereview.chromium.org/383063002/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:76: if (len >= 2
&& buffer[len-1] == '\n' && buffer[len-2] == '\r') {
Removing this makes sense because the expected string ("Test File Content", from
chrome/test/data/nacl/manifest_file/test_file.txt), does not contain any
newlines.

[hamaji, 2014-07-12 16:52:44]

> So the reason got disabled when switching to Clang wasn't anything to do
> with Clang?  Switching to Clang (for trusted code) shouldn't even affect
> the untrusted code that was buggy here.

The test was failing on non-SFI mode, where we switch to clang even for
untrusted code. Did you mean we should always use host gcc?

Anyway, landing this change wouldn't hurt even if we switch back to gcc.
I'll push this to cq.

> > As we do not use FILE* anymore, Windows specific CRLF issues should
> > not happen.
> 
> That's not quite true.  Neither fgets() nor read() will perform Windows
> CRLF conversion when running as NaCl untrusted code, which assumes Unix
> conventions.

I see. Corrected the CL description.

[hamaji, 2014-07-12 16:52:54]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-07-12 16:53:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/383063002/1

[commit-bot: I haz the power, 2014-07-12 18:31:27]

Change committed as 282838

[Mark Seaborn, 2014-07-18 00:42:36]

On 12 July 2014 09:52, <hamaji@chromium.org> wrote:

>  So the reason got disabled when switching to Clang wasn't anything to do
>> with Clang?  Switching to Clang (for trusted code) shouldn't even affect
>> the untrusted code that was buggy here.
>>
>
> The test was failing on non-SFI mode, where we switch to clang even for
> untrusted code. Did you mean we should always use host gcc?


I meant that Nico's change (
https://code.google.com/p/chromium/issues/detail?id=360311, "Switch linux
to use clang by default") only affects trusted code.  It doesn't affect
NaCl untrusted code, which is built with the NaCl toolchains.

So I don't understand why Nico's change would have affected this test, if
the change to fix the test involves changing only untrusted code.  These
manifest tests *aren't* built with the host GCC or Clang.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2014-07-18 00:42:36]

On 12 July 2014 09:52, <hamaji@chromium.org> wrote:

>  So the reason got disabled when switching to Clang wasn't anything to do
>> with Clang?  Switching to Clang (for trusted code) shouldn't even affect
>> the untrusted code that was buggy here.
>>
>
> The test was failing on non-SFI mode, where we switch to clang even for
> untrusted code. Did you mean we should always use host gcc?


I meant that Nico's change (
https://code.google.com/p/chromium/issues/detail?id=360311, "Switch linux
to use clang by default") only affects trusted code.  It doesn't affect
NaCl untrusted code, which is built with the NaCl toolchains.

So I don't understand why Nico's change would have affected this test, if
the change to fix the test involves changing only untrusted code.  These
manifest tests *aren't* built with the host GCC or Clang.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hamaji, 2014-07-23 06:56:12]

On 2014/07/18 00:42:36, Mark Seaborn wrote:
> On 12 July 2014 09:52, <mailto:hamaji@chromium.org> wrote:
> 
> >  So the reason got disabled when switching to Clang wasn't anything to do
> >> with Clang?  Switching to Clang (for trusted code) shouldn't even affect
> >> the untrusted code that was buggy here.
> >>
> >
> > The test was failing on non-SFI mode, where we switch to clang even for
> > untrusted code. Did you mean we should always use host gcc?
> 
> 
> I meant that Nico's change (
> https://code.google.com/p/chromium/issues/detail?id=360311, "Switch linux
> to use clang by default") only affects trusted code.  It doesn't affect
> NaCl untrusted code, which is built with the NaCl toolchains.
> 
> So I don't understand why Nico's change would have affected this test, if
> the change to fix the test involves changing only untrusted code.  These
> manifest tests *aren't* built with the host GCC or Clang.

Ah, I didn't realize we are using build_nexe even for non-SFI untrusted code
(except libc_free.c). The manifest tests started failing when libc_free.c
tests started failing.

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29...

So, the clang switch is still likely the cause of this. One theory is that
the clang switch has changed the usage of stack in nacl_helper. As nacl_helper
and untrusted code share the stack on non-SFI, the stack usage in trusted
code could affect the behavior of untrusted code, I think.