1. 26 Jan, 2021 1 commit
    • Wen Hui's avatar
      Sentinel: Fix Config Dependency and Rewrite Sequence (#8271) · 1aad55b6
      Wen Hui authored
      This commit fixes a well known and an annoying issue in Sentinel mode.
      
      Cause of this issue:
      Currently, Redis rewrite process works well in server mode, however in sentinel mode,
      the sentinel config has variant semantics for different configurations, in example configuration
      https://github.com/redis/redis/blob/unstable/sentinel.conf, we put comments on these.
      However the rewrite process only treat the sentinel config as a single option. During rewrite
      process, it will mess up with the lines and comments.
      
      Approaches:
      In order to solve this issue, we need to differentiate different subconfig options in sentinel separately,
      for example, sentinel monitor <master-name> <ip> <redis-port> <quorum>
      we can treat it as sentinel monitor option, instead of the sentinel option.
      
      This commit also fixes the dependency issue when putting configurations in sentinel.conf.
      For example before this commit,we must put
      `sentinel monitor <master-name> <ip> <redis-port> <quorum>` before
      `sentinel auth-pass <master-name> <password>` for a single master,
      otherwise the server cannot start and will return error. This commit fixes this issue, as long as
      the monitoring master was configured, no matter the sequence is, the sentinel can start and run properly.
      1aad55b6
  2. 20 Jan, 2021 3 commits
    • Yossi Gottlieb's avatar
      CONFIG REWRITE should honor umask settings. (#8371) · b548ffab
      Yossi Gottlieb authored
      Fixes a regression introduced due to a new (safer) way of rewriting configuration files. In the past the file was simply overwritten (same inode), but now Redis creates a new temporary file and later renames it over the old one.
      
      The temp file typically gets created with 0600 permissions so we later fchmod it to fix that. Unlike open with O_CREAT, fchmod doesn't consider umask so we have to do that explicitly.
      
      Fixes #8369
      b548ffab
    • Guy Korland's avatar
      Add CI for FreeBSD (#8292) · ac5f21d6
      Guy Korland authored
      
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      ac5f21d6
    • guybe7's avatar
      Fix firstkey,lastkey,step in COMMAND command for some commands (#8367) · baf92f3f
      guybe7 authored
      The output for COMMAND command was wrong for some commands.
      clients can use firstkey,lastkey,step to find (some) key name arguments, and the
      "movablekeys" flag to know that they can't know all (or any) of the key name arguments.
      
      These commands had the wrong output:
      1. GEORADIUS*_RO used to have "movablekeys" (which it doesn't really need)
      2. XREAD and XREADGROUP used to have (1,1,1). but that's completely wrong.
      3. Z*STORE used to have (0,0,0) but it can at lest give the index of the dstkey (1,1,1)
      baf92f3f
  3. 19 Jan, 2021 1 commit
  4. 13 Jan, 2021 1 commit
  5. 12 Jan, 2021 1 commit
    • Madelyn Olson's avatar
      Fix issues in wait test (#8310) · b24b4903
      Madelyn Olson authored
      This fixes three issues:
      1.  Using debug SLEEP was impacting the subsequent test, and causing it to pass reliably even though it should have failed. There was exactly 5 seconds of artificial pause (after 1000, wait 3000, wait 1000) between the debug sleep 5 and when we needed to unblock the client in the subsequent test. Now the test properly makes sure the client is unblocked, and the subsequent test is fixed.
      2. Minor, the client pause types were using & comparisons instead of ==, since it was previously a flag.
      3. Test is faster now that some of the hand wavy time is removed. 
      b24b4903
  6. 08 Jan, 2021 1 commit
  7. 07 Jan, 2021 4 commits
    • George Prekas's avatar
      Add check for the MADV_FREE/fork arm64 Linux kernel bug (#8224) · b02780c4
      George Prekas authored
      
      
      Older arm64 Linux kernels have a bug that could lead to data corruption during
      background save under the following scenario:
      
      1) jemalloc uses MADV_FREE on a page,
      2) jemalloc reuses and writes the page,
      3) Redis forks the background save process, and
      4) Linux performs page reclamation.
      
      Under these conditions, Linux will reclaim the page wrongfully and the
      background save process will read zeros when it tries to read the page.
      
      The bug has been fixed in Linux with commit:
      ff1712f953e27f0b0718762ec17d0adb15c9fd0b ("arm64: pgtable: Ensure dirty bit is
      preserved across pte_wrprotect()")
      
      This Commit adds an ignore-warnings config, when not found, redis will
      print a warning and exit on startup (default behavior).
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      b02780c4
    • YaacovHazan's avatar
      Report child copy-on-write info continuously · ea930a35
      YaacovHazan authored
      Add INFO field, rdb_active_cow_size, to report COW of a live fork child while
      it's active.
      - once in 1024 keys check the time, and if there's more than one second since
        the last report send a report to the parent via the pipe.
      - refactor the child_info_data struct, it's an implementation detail that
        shouldn't be in the server struct, and not used to communicate data between
        caller and callee
      - remove the magic value from that struct (not sure what it was good for), and
        instead add handling of short reads.
      - add another value to the structure, cow_type, to indicate if the report is
        for the new rdb_active_cow_size field, or it's the last report of a
        successful operation
      - add new Module API to report the active COW
      - add more asserts variants to test.tcl
      ea930a35
    • YaacovHazan's avatar
      Refactory fork child related infra, Unify child pid · f9dacf8a
      YaacovHazan authored
      This is a refactory commit, isn't suppose to have any actual impact.
      it does the following:
      - keep just one server struct fork child pid variable instead of 3
      - have one server struct variable indicating the purpose of the current fork
        child.
      - redisFork is now responsible of updating the server struct with the pid,
        which means it can be the one that calls updateDictResizePolicy
      - move child info pipe handling into redisFork instead of having them
        repeated outside
      - there are two classes of fork purposes, mutually exclusive group (AOF, RDB,
        Module), and one that can create several forks to coexist in parallel (LDB,
        but maybe Modules some day too, Module API allows for that).
      - minor fix to killRDBChild:
        unlike killAppendOnlyChild and TerminateModuleForkChild, the killRDBChild
        doesn't clear the pid variable or call wait4, so checkChildrenDone does
        the cleanup for it.
        This commit removes the explicit calls to rdbRemoveTempFile, closeChildInfoPipe,
        updateDictResizePolicy, which didn't do any harm, but where unnecessary.
      f9dacf8a
    • Jonah H. Harris's avatar
      Add ZRANGESTORE command, and improve ZSTORE command (#7844) · b5029dfd
      Jonah H. Harris authored
      
      
      Add ZRANGESTORE command, and improve ZSTORE command to deprecated Z[REV]RANGE[BYSCORE|BYLEX].
      
      Syntax for the new ZRANGESTORE command:
      ZRANGESTORE [BYSCORE | BYLEX] [REV] [LIMIT offset count]
      
      New syntax for ZRANGE:
      ZRANGE [BYSCORE | BYLEX] [REV] [WITHSCORES] [LIMIT offset count]
      
      Old syntax for ZRANGE:
      ZRANGE [WITHSCORES]
      
      Other ZRANGE commands remain unchanged.
      
      The implementation uses common code for all of these, by utilizing a consumer interface that in one
      command response to the client, and in the other command stores a zset key.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      b5029dfd
  8. 06 Jan, 2021 1 commit
    • guybe7's avatar
      Add XAUTOCLAIM (#7973) · 714e103a
      guybe7 authored
      
      
      New command: XAUTOCLAIM <key> <group> <consumer> <min-idle-time> <start> [COUNT <count>] [JUSTID]
      
      The purpose is to claim entries from a stale consumer without the usual
      XPENDING+XCLAIM combo which takes two round trips.
      
      The syntax for XAUTOCLAIM is similar to scan: A cursor is returned (streamID)
      by each call and should be used as start for the next call. 0-0 means the scan is complete.
      
      This PR extends the deferred reply mechanism for any bulk string (not just counts)
      
      This PR carries some unrelated test code changes:
      - Renames the term "client" into "consumer" in the stream-cgroups test
      - And also changes DEBUG SLEEP into "after"
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      714e103a
  9. 04 Jan, 2021 2 commits
    • Itamar Haber's avatar
      HELP subcommand, continued (#5531) · 9dcdc7e7
      Itamar Haber authored
      
      
      * man-like consistent long formatting
      * Uppercases commands, subcommands and options
      * Adds 'HELP' to HELP for all
      * Lexicographical order
      * Uses value notation and other .md likeness
      * Moves const char *help to top
      * Keeps it under 80 chars
      * Misc help typos, consistent conjuctioning (i.e return and not returns)
      * Uses addReplySubcommandSyntaxError(c) all over
      Signed-off-by: default avatarItamar Haber <itamar@redislabs.com>
      9dcdc7e7
    • Meir Shpilraien (Spielrein)'s avatar
      Fix assertion on loading AOF with timed out script. (#8284) · ecd53518
      Meir Shpilraien (Spielrein) authored
      If AOF file contains a long Lua script that timed out, then the `evalCommand` calls
      `blockingOperationEnds` which sets `server.blocked_last_cron` to 0. later on,
      the AOF `whileBlockedCron` function asserts that this value is not 0.
      
      The fix allows nesting call to `blockingOperationStarts` and `blockingOperationEnds`.
      
      The issue was first introduce in this commit: 9ef8d2f6 (Redis 6.2 RC1)
      ecd53518
  10. 03 Jan, 2021 1 commit
    • Oran Agra's avatar
      Fix rare assertion as a result of: active defrag while loading (#8281) · 7896debe
      Oran Agra authored
      In #7726 (part of 6.2), we added a mechanism for whileBlockedCron, this
      mechanism has an assertion to make sure the timestamp in
      whileBlockedCron was always set correctly before the blocking operation
      starts.
      
      I now found (thanks to our CI) two bugs in that area:
      1) CONFIG RESETSTAT (if it was allowed during loading) would have
         cleared this var
      2) the call stopLoading (which calls whileBlockedCron) was made too
         early, while the rio is still in use, in which case the update_cksum
         (rdbLoadProgressCallback) may still be called and whileBlockedCron
         can assert.
      7896debe
  11. 02 Jan, 2021 1 commit
  12. 31 Dec, 2020 1 commit
    • filipe oliveira's avatar
      Add errorstats info section, Add failed_calls and rejected_calls to commandstats (#8217) · 90b9f08e
      filipe oliveira authored
      This Commit pushes forward the observability on overall error statistics and command statistics within redis-server:
      
      It extends INFO COMMANDSTATS to have
      - failed_calls in - so we can keep track of errors that happen from the command itself, broken by command.
      - rejected_calls - so we can keep track of errors that were triggered outside the commmand processing per se
      
      Adds a new section to INFO, named ERRORSTATS that enables keeping track of the different errors that
      occur within redis ( within processCommand and call ) based on the reply Error Prefix ( The first word
      after the "-", up to the first space ).
      
      This commit also fixes RM_ReplyWithError so that it can be correctly identified as an error reply.
      90b9f08e
  13. 27 Dec, 2020 1 commit
    • Oran Agra's avatar
      Fix memory leaks in error replies due to recent change (#8249) · 049cf8cd
      Oran Agra authored
      Recently efaf09ee started using addReplyErrorSds in place of
      addReplySds the later takes ownership of the string but the former did
      not.
      This introduced memory leaks when a script returns an error to redis,
      and also in clusterRedirectClient (two new usages of
      addReplyErrorSds which was mostly unused till now.
      
      This commit chagnes two thanks.
      1. change addReplyErrorSds to take ownership of the error string.
      2. scripting.c doesn't actually need to use addReplyErrorSds, it's a
      perfect match for addReplyErrorFormat (replaces newlines with spaces)
      049cf8cd
  14. 25 Dec, 2020 1 commit
    • Itamar Haber's avatar
      Adds count to L/RPOP (#8179) · f44186e5
      Itamar Haber authored
      Adds: `L/RPOP <key> [count]`
      
      Implements no. 2 of the following strategies:
      
      1. Loop on listTypePop - this would result in multiple calls for memory freeing and allocating (see https://github.com/redis/redis/pull/8179/commits/769167a079b0e110d28e4a8099dce1ecd45682b5)
      2. Iterate the range to build the reply, then call quickListDelRange - this requires two iterations and **is the current choice**
      3. Refactor quicklist to have a pop variant of quickListDelRange - probably optimal but more complex
      
      Also:
      * There's a historical check for NULL after calling listTypePop that was converted to an assert.
      * This refactors common logic shared between LRANGE and the new form of LPOP/RPOP into addListRangeReply (adds test for b/w compat)
      * Consequently, it may have made sense to have `LRANGE l -1 -2` and `LRANGE l 9 0` be legit and return a reverse reply. Due to historical reasons that would be, however, a breaking change.
      * Added minimal comments to existing commands to adhere to the style, make core dev life easier and get commit karma, naturally.
      f44186e5
  15. 24 Dec, 2020 2 commits
  16. 23 Dec, 2020 2 commits
    • Greg Femec's avatar
      Fix random element selection for large hash tables. (#8133) · 266949c7
      Greg Femec authored
      
      
      When a database on a 64 bit build grows past 2^31 keys, the underlying hash table expands to 2^32 buckets. After this point, the algorithms for selecting random elements only return elements from half of the available buckets because they use random() which has a range of 0 to 2^31 - 1. This causes problems for eviction policies which use dictGetSomeKeys or dictGetRandomKey. Over time they cause the hash table to become unbalanced because, while new keys are spread out evenly across all buckets, evictions come from only half of the available buckets. Eventually this half of the table starts to run out of keys and it takes longer and longer to find candidates for eviction. This continues until no more evictions can happen.
      
      This solution addresses this by using a 64 bit PRNG instead of libc random().
      Co-authored-by: default avatarGreg Femec <gfemec@google.com>
      266949c7
    • Oran Agra's avatar
      fix valgrind warning created by recent pidfile fix (#8235) · 2426aaa0
      Oran Agra authored
      This isn't a leak, just an warning due to unreachable
      allocation on the fork child.
      Problem created by 92a483bc
      2426aaa0
  17. 22 Dec, 2020 2 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Fix issue where fork process deletes the parent pidfile (#8231) · 92a483bc
      Meir Shpilraien (Spielrein) authored
      Turns out that when the fork child crashes, the crash log was deleting
      the pidfile from the disk (although the parent is still running.
      
      Now we set the pidfile of the fork process to NULL so the fork process
      will never deletes it.
      92a483bc
    • Oran Agra's avatar
      Remove read-only flag from non-keyspace cmds, different approach for EXEC to... · 411c18bb
      Oran Agra authored
      Remove read-only flag from non-keyspace cmds, different approach for EXEC to propagate MULTI (#8216)
      
      In the distant history there was only the read flag for commands, and whatever
      command that didn't have the read flag was a write one.
      Then we added the write flag, but some portions of the code still used !read
      Also some commands that don't work on the keyspace at all, still have the read
      flag.
      
      Changes in this commit:
      1. remove the read-only flag from TIME, ECHO, ROLE and LASTSAVE
      
      2. EXEC command used to decides if it should propagate a MULTI by looking at
         the command flags (!read & !admin).
         When i was about to change it to look at the write flag instead, i realized
         that this would cause it not to propagate a MULTI for PUBLISH, EVAL, and
         SCRIPT, all 3 are not marked as either a read command or a write one (as
         they should), but all 3 are calling forceCommandPropagation.
      
         So instead of introducing a new flag to denote a command that "writes" but
         not into the keyspace, and still needs propagation, i decided to rely on
         the forceCommandPropagation, and just fix the code to propagate MULTI when
         needed rather than depending on the command flags at all.
      
         The implication of my change then is that now it won't decide to propagate
         MULTI when it sees one of these: SELECT, PING, INFO, COMMAND, TIME and
         other commands which are neither read nor write.
      
      3. Changing getNodeByQuery and clusterRedirectBlockedClientIfNeeded in
         cluster.c to look at !write rather than read flag.
         This should have no implications, since these code paths are only reachable
         for commands which access keys, and these are always marked as either read
         or write.
      
      This commit improve MULTI propagation tests, for modules and a bunch of
      other special cases, all of which used to pass already before that commit.
      the only one that test change that uncovered a change of behavior is the
      one that DELs a non-existing key, it used to propagate an empty
      multi-exec block, and no longer does.
      411c18bb
  18. 21 Dec, 2020 1 commit
    • valentinogeron's avatar
      Fix PFDEBUG commands flag (#8222) · 4c13945c
      valentinogeron authored
      - Mark it as a @hyperloglog command (ACL)
      - Should not be allowed in OOM
      - Add firstkey, lastkey, step
      - Add comment that explains the 'write' flag
      4c13945c
  19. 18 Dec, 2020 1 commit
  20. 13 Dec, 2020 2 commits
    • filipe oliveira's avatar
      Expose Redis main thread cpu time, and scrape system time via INFO command. (#8132) · 19d46f8f
      filipe oliveira authored
      Exposes the main thread CPU info via info modules ( linux specific only )
      (used_cpu_sys_main_thread and used_cpu_user_main_thread). This is important for:
      
      - distinguish between main thread and io-threads cpu time total cpu time consumed ( check
        what is the first bottleneck on the used config )
      - distinguish between main thread and modules threads total cpu time consumed
      
      Apart from it, this commit also exposes the server_time_usec within the Server section so that we can
      properly differentiate consecutive collection and calculate for example the CPU% and or / cpu time vs
      wall time, etc...
      19d46f8f
    • Wang Yuan's avatar
      Add total_forks to INFO STATS (#8155) · e3ff4145
      Wang Yuan authored
      e3ff4145
  21. 12 Dec, 2020 1 commit
    • 杨博东's avatar
      Add GEOSEARCH / GEOSEARCHSTORE commands (#8094) · 4d06d99b
      杨博东 authored
      Add commands to query geospatial data with bounding box.
      
      Two new commands that replace the existing 4 GEORADIUS* commands.
      
      GEOSEARCH key [FROMMEMBER member] [FROMLOC long lat] [BYRADIUS radius
      unit] [BYBOX width height unit] [WITHCORD] [WITHDIST] [WITHASH] [COUNT
      count] [ASC|DESC]
      
      GEOSEARCHSTORE dest_key src_key [FROMMEMBER member] [FROMLOC long lat]
      [BYRADIUS radius unit] [BYBOX width height unit] [WITHCORD] [WITHDIST]
      [WITHASH] [COUNT count] [ASC|DESC] [STOREDIST]
      
      - Add two types of CIRCULAR_TYPE and RECTANGLE_TYPE to achieve different searches
      - Judge whether the point is within the rectangle, refer to:
      geohashGetDistanceIfInRectangle
      4d06d99b
  22. 06 Dec, 2020 4 commits
    • Oran Agra's avatar
      Sanitize dump payload: fuzz tester and fixes for segfaults and leaks it exposed · c31055db
      Oran Agra authored
      The test creates keys with various encodings, DUMP them, corrupt the payload
      and RESTORES it.
      It utilizes the recently added use-exit-on-panic config to distinguish between
       asserts and segfaults.
      If the restore succeeds, it runs random commands on the key to attempt to
      trigger a crash.
      
      It runs in two modes, one with deep sanitation enabled and one without.
      In the first one we don't expect any assertions or segfaults, in the second one
      we expect assertions, but no segfaults.
      We also check for leaks and invalid reads using valgrind, and if we find them
      we print the commands that lead to that issue.
      
      Changes in the code (other than the test):
      - Replace a few NPD (null pointer deference) flows and division by zero with an
        assertion, so that it doesn't fail the test. (since we set the server to use
        `exit` rather than `abort` on assertion).
      - Fix quite a lot of flows in rdb.c that could have lead to memory leaks in
        RESTORE command (since it now responds with an error rather than panic)
      - Add a DEBUG flag for SET-SKIP-CHECKSUM-VALIDATION so that the test don't need
        to bother with faking a valid checksum
      - Remove a pile of code in serverLogObjectDebugInfo which is actually unsafe to
        run in the crash report (see comments in the code)
      - fix a missing boundary check in lzf_decompress
      
      test suite infra improvements:
      - be able to run valgrind checks before the process terminates
      - rotate log files when restarting servers
      c31055db
    • Oran Agra's avatar
      Sanitize dump payload: ziplist, listpack, zipmap, intset, stream · ca1c1825
      Oran Agra authored
      When loading an encoded payload we will at least do a shallow validation to
      check that the size that's encoded in the payload matches the size of the
      allocation.
      This let's us later use this encoded size to make sure the various offsets
      inside encoded payload don't reach outside the allocation, if they do, we'll
      assert/panic, but at least we won't segfault or smear memory.
      
      We can also do 'deep' validation which runs on all the records of the encoded
      payload and validates that they don't contain invalid offsets. This lets us
      detect corruptions early and reject a RESTORE command rather than accepting
      it and asserting (crashing) later when accessing that payload via some command.
      
      configuration:
      - adding ACL flag skip-sanitize-payload
      - adding config sanitize-dump-payload [yes/no/clients]
      
      For now, we don't have a good way to ensure MIGRATE in cluster resharding isn't
      being slowed down by these sanitation, so i'm setting the default value to `no`,
      but later on it should be set to `clients` by default.
      
      changes:
      - changing rdbReportError not to `exit` in RESTORE command
      - adding a new stat to be able to later check if cluster MIGRATE isn't being
        slowed down by sanitation.
      ca1c1825
    • guybe7's avatar
      Make sure we do not propagate nested MULTI/EXEC (#8097) · 1df5bb56
      guybe7 authored
      One way this was happening is when a module issued an RM_Call which would inject MULTI.
      If the module command that does that was itself issued by something else that already did
      added MULTI (e.g. another module, or a Lua script), it would have caused nested MULTI.
      
      In fact the MULTI state in the client or the MULTI_EMITTED flag in the context isn't
      the right indication that we need to propagate MULTI or not, because on a nested calls
      (possibly a module action called by a keyspace event of another module action), these
      flags aren't retained / reflected.
      
      instead there's now a global propagate_in_transaction flag for that.
      
      in addition to that, we now have a global in_eval and in_exec flags, to serve the flags
      of RM_GetContextFlags, since their dependence on the current client is wrong for the same
      reasons mentioned above.
      1df5bb56
    • Wang Yuan's avatar
      Limit the main db and expires dictionaries to expand (#7954) · 75f9dec6
      Wang Yuan authored
      As we know, redis may reject user's requests or evict some keys if
      used memory is over maxmemory. Dictionaries expanding may make
      things worse, some big dictionaries, such as main db and expires dict,
      may eat huge memory at once for allocating a new big hash table and be
      far more than maxmemory after expanding.
      There are related issues: #4213 #4583
      
      More details, when expand dict in redis, we will allocate a new big
      ht[1] that generally is double of ht[0], The size of ht[1] will be
      very big if ht[0] already is big. For db dict, if we have more than
      64 million keys, we need to cost 1GB for ht[1] when dict expands.
      
      If the sum of used memory and new hash table of dict needed exceeds
      maxmemory, we shouldn't allow the dict to expand. Because, if we
      enable keys eviction, we still couldn't add much more keys after
      eviction and rehashing, what's worse, redis will keep less keys when
      redis only remains a little memory for storing new hash table instead
      of users' data. Moreover users can't write data in redis if disable
      keys eviction.
      
      What this commit changed ?
      
      Add a new member function expandAllowed for dict type, it provide a way
      for caller to allow expand or not. We expose two parameters for this
      function: more memory needed for expanding and dict current load factor,
      users can implement a function to make a decision by them.
      For main db dict and expires dict type, these dictionaries may be very
      big and cost huge memory for expanding, so we implement a judgement
      function: we can stop dict to expand provisionally if used memory will
      be over maxmemory after dict expands, but to guarantee the performance
      of redis, we still allow dict to expand if dict load factor exceeds the
      safe load factor.
      Add test cases to verify we don't allow main db to expand when left
      memory is not enough, so that avoid keys eviction.
      
      Other changes:
      
      For new hash table size when expand. Before this commit, the size is
      that double used of dict and later _dictNextPower. Actually we aim to
      control a dict load factor between 0.5 and 1.0. Now we replace *2 with
      +1, since the first check is that used >= size, the outcome of before
      will usually be the same as _dictNextPower(used+1). The only case where
      it'll differ is when dict_can_resize is false during fork, so that later
      the _dictNextPower(used*2) will cause the dict to jump to *4 (i.e.
      _dictNextPower(1025*2) will return 4096).
      Fix rehash test cases due to changing algorithm of new hash table size
      when expand.
      75f9dec6
  23. 01 Dec, 2020 1 commit
    • Itamar Haber's avatar
      Adds pub/sub channel patterns to ACL (#7993) · c1b1e8c3
      Itamar Haber authored
      Fixes #7923.
      
      This PR appropriates the special `&` symbol (because `@` and `*` are taken),
      followed by a literal value or pattern for describing the Pub/Sub patterns that
      an ACL user can interact with. It is similar to the existing key patterns
      mechanism in function (additive) and implementation (copy-pasta). It also adds
      the allchannels and resetchannels ACL keywords, naturally.
      
      The default user is given allchannels permissions, whereas new users get
      whatever is defined by the acl-pubsub-default configuration directive. For
      backward compatibility in 6.2, the default of this directive is allchannels but
      this is likely to be changed to resetchannels in the next major version for
      stronger default security settings.
      
      Unless allchannels is set for the user, channel access permissions are checked
      as follows :
      * Calls to both PUBLISH and SUBSCRIBE will fail unless a pattern matching the
        argumentative channel name(s) exists for the user.
      * Calls to PSUBSCRIBE will fail unless the pattern(s) provided as an argument
        literally exist(s) in the user's list.
      
      Such failures are logged to the ACL log.
      
      Runtime changes to channel permissions for a user with existing subscribing
      clients cause said clients to disconnect unless the new permissions permit the
      connections to continue. Note, however, that PSUBSCRIBErs' patterns are matched
      literally, so given the change bar:* -> b*, pattern subscribers to bar:* will be
      disconnected.
      
      Notes/questions:
      * UNSUBSCRIBE, PUNSUBSCRIBE and PUBSUB remain unprotected due to lack of reasons
        for touching them.
      c1b1e8c3
  24. 25 Nov, 2020 1 commit
    • Oran Agra's avatar
      INFO client_recent_max_input_buffer includes argv array (#8065) · bbc2c445
      Oran Agra authored
      this metric already includes the argv bytes, like what clientsCronTrackClientsMemUsage does, but it's missing the array itself.
      
      p.s. For the purpose of tracking expensive clients we don't need to include the size of the client struct and the static reply buffer in it.
      bbc2c445
  25. 22 Nov, 2020 2 commits
    • Oran Agra's avatar
      Fix bug with module GIL being released prematurely (#8061) · e6fa4738
      Oran Agra authored
      This is hopefully usually harmles.
      The server.ready_keys will usually be empty so the code after releasing
      the GIL will soon be done.
      The only case where it'll actually process things is when a module
      releases a client (or module) blocked on a key, by triggering this NOT
      from within a command (e.g. a timer event).
      
      This bug was introduced in redis 6.0.9, see #7903
      e6fa4738
    • Oran Agra's avatar
      Fix oom-score-adj-values range, abs options, and bug when used in config file (#8046) · 61954951
      Oran Agra authored
      Fix: When oom-score-adj-values is provided in the config file after
      oom-score-adj yes, it'll take an immediate action, before
      readOOMScoreAdj was acquired, resulting in an error (out of range score
      due to uninitialized value. delay the reaction the real call is made by
      main().
      
      Since the values are clamped to -1000..1000, and they're
      applied as an offset from the value at startup (which may be -1000), we
      need to allow the offsets to reach to +2000 so that a value of +1000 is
      achievable in case the value at startup was -1000.
      
      Adding an option for absolute values rather than relative ones.
      61954951
  26. 17 Nov, 2020 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Unified MULTI, LUA, and RM_Call with respect to blocking commands (#8025) · d87a0d02
      Meir Shpilraien (Spielrein) authored
      
      
      Blocking command should not be used with MULTI, LUA, and RM_Call. This is because,
      the caller, who executes the command in this context, expects a reply.
      
      Today, LUA and MULTI have a special (and different) treatment to blocking commands:
      
      LUA   - Most commands are marked with no-script flag which are checked when executing
      and command from LUA, commands that are not marked (like XREAD) verify that their
      blocking mode is not used inside LUA (by checking the CLIENT_LUA client flag).
      MULTI - Command that is going to block, first verify that the client is not inside
      multi (by checking the CLIENT_MULTI client flag). If the client is inside multi, they
      return a result which is a match to the empty key with no timeout (for example blpop
      inside MULTI will act as lpop)
      For modules that perform RM_Call with blocking command, the returned results type is
      REDISMODULE_REPLY_UNKNOWN and the caller can not really know what happened.
      
      Disadvantages of the current state are:
      
      No unified approach, LUA, MULTI, and RM_Call, each has a different treatment
      Module can not safely execute blocking command (and get reply or error).
      Though It is true that modules are not like LUA or MULTI and should be smarter not
      to execute blocking commands on RM_Call, sometimes you want to execute a command base
      on client input (for example if you create a module that provides a new scripting
      language like javascript or python).
      While modules (on modules command) can check for REDISMODULE_CTX_FLAGS_LUA or
      REDISMODULE_CTX_FLAGS_MULTI to know not to block the client, there is no way to
      check if the command came from another module using RM_Call. So there is no way
      for a module to know not to block another module RM_Call execution.
      
      This commit adds a way to unify the treatment for blocking clients by introducing
      a new CLIENT_DENY_BLOCKING client flag. On LUA, MULTI, and RM_Call the new flag
      turned on to signify that the client should not be blocked. A blocking command
      verifies that the flag is turned off before blocking. If a blocking command sees
      that the CLIENT_DENY_BLOCKING flag is on, it's not blocking and return results
      which are matches to empty key with no timeout (as MULTI does today).
      
      The new flag is checked on the following commands:
      
      List blocking commands: BLPOP, BRPOP, BRPOPLPUSH, BLMOVE,
      Zset blocking commands: BZPOPMIN, BZPOPMAX
      Stream blocking commands: XREAD, XREADGROUP
      SUBSCRIBE, PSUBSCRIBE, MONITOR
      In addition, the new flag is turned on inside the AOF client, we do not want to
      block the AOF client to prevent deadlocks and commands ordering issues (and there
      is also an existing assert in the code that verifies it).
      
      To keep backward compatibility on LUA, all the no-script flags on existing commands
      were kept untouched. In addition, a LUA special treatment on XREAD and XREADGROUP was kept.
      
      To keep backward compatibility on MULTI (which today allows SUBSCRIBE, and PSUBSCRIBE).
      We added a special treatment on those commands to allow executing them on MULTI.
      
      The only backward compatibility issue that this PR introduces is that now MONITOR
      is not allowed inside MULTI.
      
      Tests were added to verify blocking commands are not blocking the client on LUA, MULTI,
      or RM_Call. Tests were added to verify the module can check for CLIENT_DENY_BLOCKING flag.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarItamar Haber <itamar@redislabs.com>
      d87a0d02