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

Fix logger overwriting vars in some circumstances #32898

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 16, 2022

We were investigating one case, indicated by the error foreach() argument must be of type array|object, string given

This can be reproduce like this:

1.) in php.ini, zend.exception_ignore_args is set to "Off" (PHP default, check distro in doubt)
2.) app files_lock enabled
3.) use an addressbook client (like KAddressbook)
4.) there, add a new contact

You will see an exception like:

{
  "Exception": "Error",
  "Message": "Invalid argument supplied for foreach() at /srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php#1454",
  "Code": 0,
  "Trace": [
    {
      "file": "/srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php",
      "line": 1454,
      "function": "onAll",
      "class": "OC\\Log\\ErrorHandler",
      "type": "::",
      "args": [
        2,
        "Invalid argument supplied for foreach()",
        "/srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php",
        1454,
        {
          "request": {
            "__class__": "Sabre\\HTTP\\Request"
          },
          "response": {
            "__class__": "Sabre\\HTTP\\Response"
          },
          "path": "addressbooks/users/stable24/contacts/1655379707.R254.vcf",
          "node": null,
          "lastMod": null,
          "etag": null,
          "ifMatch": null,
          "e": {
            "__class__": "Sabre\\DAV\\Exception\\NotFound"
          },
          "ifNoneMatch": "*",
          "nodeExists": false,
          "ifUnmodifiedSince": null,
          "ifConditions": "*** sensitive parameter replaced ***"
        }
      ]
    },
    {
      "file": "/srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php",
      "line": 466,
      "function": "checkPreconditions",
      "class": "Sabre\\DAV\\Server",
      "type": "->",
      "args": [
        {
          "__class__": "Sabre\\HTTP\\Request"
        },
        {
          "__class__": "Sabre\\HTTP\\Response"
        }
      ]
    },
    {
      "file": "/srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php",
      "line": 253,
      "function": "invokeMethod",
      "class": "Sabre\\DAV\\Server",
      "type": "->",
      "args": [
        {
          "__class__": "Sabre\\HTTP\\Request"
        },
        {
          "__class__": "Sabre\\HTTP\\Response"
        }
      ]
    },
    {
      "file": "/srv/http/nextcloud/stable24/3rdparty/sabre/dav/lib/DAV/Server.php",
      "line": 321,
      "function": "start",
      "class": "Sabre\\DAV\\Server",
      "type": "->",
      "args": []
    },
    {
      "file": "/srv/http/nextcloud/stable24/apps/dav/lib/Server.php",
      "line": 352,
      "function": "exec",
      "class": "Sabre\\DAV\\Server",
      "type": "->",
      "args": []
    },
    {
      "file": "/srv/http/nextcloud/stable24/apps/dav/appinfo/v2/remote.php",
      "line": 35,
      "function": "exec",
      "class": "OCA\\DAV\\Server",
      "type": "->",
      "args": []
    },
    {
      "file": "/srv/http/nextcloud/stable24/remote.php",
      "line": 166,
      "args": [
        "/srv/http/nextcloud/stable24/apps/dav/appinfo/v2/remote.php"
      ],
      "function": "require_once"
    }
  ],
  "File": "/srv/http/nextcloud/stable24/lib/private/Log/ErrorHandler.php",
  "Line": 99,
  "CustomMessage": "--"
}

The hidden issue is that a variable, passed by reference, was overwritten. I used to be an array and was turned into a string *** sensitive parameter replaced ***.

Now we have a test to reproduce the issue and the fix.

This problem popped up in a related scenario, where we committed a temporary fix: #32685

@blizzz blizzz added bug 3. to review Waiting for reviews labels Jun 16, 2022
@blizzz blizzz added this to the Nextcloud 25 milestone Jun 16, 2022
@blizzz blizzz requested review from mgallien, ChristophWurst, miaulalala, a team, icewind1991, CarlSchwan and come-nc and removed request for a team June 16, 2022 11:46
@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

/backport to stable25

@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

/backport to stable24

@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

/backport to stable23

@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

special thanks to @miaulalala and @mgallien for the debugging sessions <3

@blizzz blizzz force-pushed the fix/noid/logger-overwrites-vars branch from 5da846c to 52839f6 Compare June 16, 2022 11:57
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Nasty one

@blizzz blizzz force-pushed the fix/noid/logger-overwrites-vars branch 2 times, most recently from 866baac to 7d443de Compare June 16, 2022 15:48
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

nice!

@blizzz blizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 16, 2022
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the fix/noid/logger-overwrites-vars branch from 7d443de to 8b2b594 Compare June 16, 2022 16:38
@blizzz blizzz merged commit 6929438 into master Jun 16, 2022
@blizzz blizzz deleted the fix/noid/logger-overwrites-vars branch June 16, 2022 18:32
@backportbot-nextcloud
Copy link

The backport to stable25 failed. Please do this backport manually.

@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

/backport to stable22

@blizzz
Copy link
Member Author

blizzz commented Jun 16, 2022

The backport to stable25 failed. Please do this backport manually.

off by one 🤭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants