Skip to content

Specify -fno-sanitize-recover #4672

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Specify -fno-sanitize-recover #4672

wants to merge 5 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 3, 2019

And backport changes to symfony setup from #4666.

@nikic
Copy link
Member Author

nikic commented Sep 3, 2019

/home/vsts/work/1/s/Zend/zend_execute_API.c:683:45: runtime error: member access within misaligned address 0x000000000002 for type 'const struct zend_op', which requires 8 byte alignment
0x000000000002: note: pointer points here

Not seeing this one locally...

@nikic
Copy link
Member Author

nikic commented Sep 3, 2019

Annoyingly the issue disappears when specifying --debug...

@nikic
Copy link
Member Author

nikic commented Sep 3, 2019

Stack trace from asan:

ASAN:SIGSEGV
=================================================================
==74617==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001e (pc 0x000001b8df56 bp 0x7ffec151e280 sp 0x7ffec151e080 T0)
    #0 0x1b8df55 in zend_call_function /home/vsts/work/1/s/Zend/zend_execute_API.c:683
    #1 0x1d0bcae in zend_objects_destroy_object /home/vsts/work/1/s/Zend/zend_objects.c:179
    #2 0x1cc3d0a in zend_gc_collect_cycles /home/vsts/work/1/s/Zend/zend_gc.c:1537
    #3 0x1cb9681 in gc_possible_root_when_full /home/vsts/work/1/s/Zend/zend_gc.c:592
    #4 0x1cb9e28 in gc_possible_root /home/vsts/work/1/s/Zend/zend_gc.c:642
    #5 0x1d50bb8 in gc_check_possible_root /home/vsts/work/1/s/Zend/zend_gc.h:83
    #6 0x1d790a8 in i_free_compiled_variables /home/vsts/work/1/s/Zend/zend_execute.c:3393
    #7 0x1f5e13d in execute_ex /home/vsts/work/1/s/Zend/zend_vm_execute.h:53342
    #8 0x1f79ab4 in zend_execute /home/vsts/work/1/s/Zend/zend_vm_execute.h:57561
    #9 0x1bf1580 in zend_execute_scripts /home/vsts/work/1/s/Zend/zend.c:1663
    #10 0x198c2ab in php_execute_script /home/vsts/work/1/s/main/main.c:2619
    #11 0x1f82556 in do_cli /home/vsts/work/1/s/sapi/cli/php_cli.c:961
    #12 0x1f8545c in main /home/vsts/work/1/s/sapi/cli/php_cli.c:1352
    #13 0x7fd34c14882f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #14 0x452c18 in _start (/usr/bin/php+0x452c18)

@nikic
Copy link
Member Author

nikic commented Sep 4, 2019

Looks like the hanging test is Symfony\Bridge\Twig\Tests\Command\LintCommandTest::testLintDefaultPaths. @nicolas-grekas Any off-hand ideas on why that might happen?

@nikic
Copy link
Member Author

nikic commented Sep 5, 2019

Looks like the test was only added two days ago: symfony/symfony@1bfefef

As the code involves STDIN, I'll try closing that again...

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Sep 5, 2019 •
edited
Loading

Thanks for investigating. Reported in symfony/symfony#33446 (comment)

@nicolas-grekas nicolas-grekas mentioned this pull request Sep 5, 2019
Merged
@nikic
Copy link
Member Author

nikic commented Sep 5, 2019

Okay, closing STDIN did not help...

PHP's behavior wrt stdin seems to be pretty weird though. I tried this code:

<?php
  
var_dump(STDIN);
var_dump(ftell(STDIN));
var_dump(feof(STDIN));
var_dump(fread(STDIN, 1));

When run with php test.php 0<&- I get:

resource(1) of type (stream)
int(0)
bool(false)

// Notice: fread(): read of 8192 bytes failed with errno=9 Bad file descriptor in /home/nikic/php-7.4/t147.php on line 6
bool(false)

Not quite sure what should be happening here, but possibly feof() should return true?

There's also this concerning bug report: https://bugs.php.net/bug.php?id=74252

@yceruto yceruto mentioned this pull request Sep 6, 2019
Merged
fabpot added a commit to symfony/symfony that referenced this pull request Sep 8, 2019
@fabpot
feature #33496 Deprecated not passing dash symbol (-) to STDIN comman…
4234601 
…ds (yceruto)

This PR was merged into the 4.4 branch.

Discussion
----------

Deprecated not passing dash symbol (-) to STDIN commands

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #33446 (comment)
| License       | MIT
| Doc PR        | -

Follow-up #33446

> There's a conflict here: when no argument was provided, the command also reads from STDIN.
So now, it reads from STDIN, and if there is nothing there, reads from the default template.
This has been caught in php/php-src#4672

> This creates an ambiguous situation - maybe one did pipe nothing but doesn't expect the default template dir to be linted.

> I'd suggest resolving the ambiguity by reading from STDIN only when explicitly asked for. Passing - as argument could the way. And we could trigger a deprecation for now.

For consistency, the other 2 lint commands (`lint:yaml` and `lint:xliff`) have been touched as well.

The plan for 5.0 is read from the STDIN only when `-` is given.

/cc @nicolas-grekas

Commits
-------

586f299 deprecated not passing dash symbol (-) to STDIN commands
@nikic
Copy link
Member Author

nikic commented Sep 11, 2019

This nearly works, but something is still doing an exit(1) despite the applied phpunit patch ...

nicolas-grekas and others added 5 commits September 11, 2019 11:15
@nicolas-grekas @nikic
Improve Symfony test setup
aee31dd 
Perform PHPUnit installation under php7.3 -- which will also make
it work on master.

Also properly resolve the tty issue by specifying excluded groups
during the test run.
@nikic
Specify -fsanitize-no-recover
9048e2d
@nikic
Run community job
f9a7280
@nikic
Apply phpunit exit patch for symfony as well
6c1ded7 
Now that phpunit is installed separately, we can do this and not
worry about managing exit codes.
@nikic
Revert "Apply phpunit exit patch for symfony as well"
2c837d1 
This reverts commit 6c1ded7.
@nikic
Copy link
Member Author

nikic commented Sep 11, 2019

Merged while keeping the current exit code workaround...

@nikic nikic closed this Sep 11, 2019
@nikic nikic mentioned this pull request Sep 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nikic @nicolas-grekas