laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.55k stars 11.03k forks source link

Notice: unserialize(): Error at offset 0 of 125 bytes #27651

Closed danijar2000 closed 5 years ago

danijar2000 commented 5 years ago

Description:

in File: https://github.com/laravel/framework/blob/5.7/src/Illuminate/Auth/Recaller.php#L24

$this->recaller = @unserialize($recaller, ['allowed_classes' => false]) ?: $recaller;

$recaller value ever time like this

1|HlwYAkqrxAtvRaMGoGoCHeFGzWsqSorUH14Kt5xcG1jDFRRFx5kJJjX933Lc|$2y$10$5UvGR29rJHeP.NCpi4D3BexFgQHlV7uKVK3jH4kYKEeHKOIBIsbfW

And ever time send report Sentry.io

Notice: unserialize(): Error at offset 0 of 125 bytes

Steps To Reproduce:

You can insert in file: https://github.com/laravel/framework/blob/5.7/src/Illuminate/Auth/SessionGuard.php#L134 Code like this: $this->user = null;

And install bug tracker https://github.com/getsentry/sentry-laravel

driesvints commented 5 years ago

I'll need more info and/or code to debug this further. Please post relevant code like models, jobs, commands, notifications, events, listeners, controller methods, routes, etc. You may use https://paste.laravel.io to post larger snippets or just reply with shorter code snippets. Thanks!

danijar2000 commented 5 years ago

1) Install new project.

$ composer create-project --prefer-dist laravel/laravel test

$ cd test

2) Make Auth.

$ php artisan make:auth

3) Configuration .env file

DB_DATABASE=test
DB_USERNAME=root
DB_PASSWORD=password

4) Edit seed classes database\seeds\DatabaseSeeder.php

<?php
use Illuminate\Database\Seeder;
class DatabaseSeeder extends Seeder
{
    public function run()
    {
        DB::table('users')->insert([
            'name' => 'test',
            'email' => 'test@gmail.com',
            'password' => bcrypt('test'),
        ]);
    }
}

5) Migrate and Seed

$ php artisan migrate --seed

6) Create project in sentry.io 7) Install the sentry/sentry-laravel package: https://docs.sentry.io/clients/php/integrations/laravel/ 8) Run serve

$ php artisan serve

9) Go to url: http://localhost:8000/login 10) Login Attention! Checked

Remember Me

11) Logout.

12) Open project in sentry.io. You will see notice:

Notice: unserialize(): Error at offset 0 of 123 bytes

driesvints commented 5 years ago

I can't reproduce this on a fresh Laravel install. I believe something might be wrong with your PHP setup. Can you first please try one of the following support channels? If you can actually identify this as a bug, feel free to report back.

jessarcher commented 5 years ago

FWIW I'm having this same issue in 5.8.17 with alerts being sent to HoneyBadger.

I believe the fact that we can see it in sentry and HoneyBadger is related to the fact that the unserialize() call is prefixed with the @ error control operator. I.e.

$this->recaller = @unserialize($recaller, ['allowed_classes' => false]) ?: $recaller;

I am assuming that the error catching hooks being used by sentry and HoneyBadger are picking up the error, regardless of that operator, which is otherwise being ignored by the framework.

If I remove the @ then I able to see the unserialize() errors in the Laravel logs as well. Presumably they are expected and intentionally ignored.

danijar2000 commented 5 years ago

This bug disappeared from getsentry / sentry-laravel version 1.0.1. In version 1.0.0-beta5, this error is still there. I can not determine what exactly they fixed in Sentry-laravel. I think HoneyBadger should fix this too.

On good, Laravel developers should have fixed this bug. But I think this is difficult to fix. Since the extra code to validate the input data will slow down the framework. Is that bad.

CaelanStewart commented 4 years ago

I keep getting similar errors using Redis cache in the Vagrant/Homestead box (stock, no mods).

@danijar2000 - are you using redis driver for sessions, with the redis client set to predis?

I get the same error, though not with sessions. Instead it's a bit of code that interacts with Laravel's Cache abstraction directly, rather than emanating from an internal Laravel service.

This may be related if Laravel's redis session driver does not use the framework's own cache abstraction (RedisStore), because if it did, the error would have emanated from the same point as it did for me. I can see this being possible if the session must be determined before the cache service is ready. I'm not too familiar with Laravel internals.

ErrorException(code: 0): unserialize(): Error at offset 0 of 16 bytes at /home/vagrant/code/project/vendor/laravel/framework/src/Illuminate/Cache/RedisStore.php:306

If I log out the value before the cache item is unserialised in the following method (RedisStore.php:304), it does appear to be a random string, instead of a serialised array, which went in to begin with (though Laravel's cache abstraction did the serialisation, and it only puts the value raw if it's numeric):

/**
     * Unserialize the value.
     *
     * @param  mixed  $value
     * @return mixed
     */
    protected function unserialize($value)
    {
        log_debug($value);
        return is_numeric($value) ? $value : unserialize($value);
    }

But, looking through RedisStore.php, there's very little abstraction over the \Predis interface. And no abstraction that could end up with a cache key resolving to an apparently random string.

So perhaps there's a bug in either predis or the PHP Redis extension? I'd bet on predis, since Laravel documents that it is abandoned and support may be dropped in a future version.

An important note about my case: I've noticed this happen only where I've had to use cache locks. This has never happened to cache keys that were not locked before accessing them.

I've switched from predis to phpredis. So we'll see what happens.

danijar2000 commented 4 years ago

@CaelanStewart No, this is not related to redis or any cache. You can check by removing the @ character before the unserialize function.

CaelanStewart commented 4 years ago

@CaelanStewart No, this is not related to redis or any cache. You can check by removing the @ character before the unserialize function.

Perhaps it is not. And yes, I am aware of the function of @, and that the error emanated from a different point for you. Which is why I posited that Laravel's redis session driver does not use the RedisStore internally. If it did not, then this would be possible.

However, Laravel does have a redis driver for sessions. So it is certainly possible that this was caused by redis at some point along the line.

That's why I asked if you used the redis driver for sessions.

nickpoulos commented 4 years ago

@CaelanStewart were you every able to resolve this? I am using PHP7.4, Laravel 7.0+.

I am having am an identical problem to you. When trying to Cache::get(), only when using Redis locks, I am getting:

ErrorException unserialize(): Error at offset 0 of 16 bytes

I also dumped out the value before the cache item is unserialized in RedisStore, and out comes a random string, instead of a serialized array that went in.

If I do a regular Cache::get() without locks, all works as expected.

CaelanStewart commented 4 years ago

@nickpoulos - I wasn't able to solve the issue, though I could work around it.

I created a simple wrapper class for cache locks which appends a string (something like "-mutex") to cache key, and creates a new cache item, which contains a boolean representing the lock state.

You might want to add a try catch, and rethrow the exception in the catch block, but in the finally block, forget the cache lock state key, to ensure no locks are accidentally kept if an exception is thrown.

renta commented 4 years ago

An easy way to reproduce this bug:

So, cache store would attempt to unserialize a simple string which would lead to PHP notice and false result.

exzachlyvv commented 4 years ago

I recently ran into this issue. The reason it was occurring is because I was using the same cache key for the ->lock($key) and the ->get($key). To solve it, I used a different key and did as @CaelanStewart suggested: ->lock("{$key}-mutex")

jbreuer95 commented 3 years ago

An easy way to reproduce this bug:

  • Create a lock with Laravel atomic lock. It will create a string key in the Redis DB.
  • On the next step you want to check that your entity is locked (in separate request), but Laravel atomic locks do not have such methods, so you decide to just retrieve the key from Redis DB with cache->get($key) method. But because it would be processed by this code in RedisStore.php (line 306):
    public function get($key)
    {
        $value = $this->connection()->get($this->prefix.$key);

        return ! is_null($value) ? $this->unserialize($value) : null;
    }

So, cache store would attempt to unserialize a simple string which would lead to PHP notice and false result.

I ran into this and found a workaround:

Cache::lockConnection()->get(Cache::getPrefix().$key)