Hello, this is just a ping for @oranagra and @soloestoy in case of tracing bugs in Redis OSS or in other private forks. Recently I realized that before Redis 4.0, which uses as shared objects a special refcount of INT_MAX, among the integer overflows already fixed we could also run into the overflowing of the refcount of small integers objects. This could happen even without triggering the old dict.c overflow problem, just creating many fairly large hashes, with many fields set to the same shared object, like the string "1". The refcount will overflow at some point, later decrRefCount() could be called detecting a refcount <= 0, and aborting for the assert as a result. I believe there is nothing to fix because of the shared objects new semantics... but I wonder if you ran into this. Thanks.
Comment From: oranagra
i haven't seen it.. what you say that in totaly we'll have more than 2 million fields with the same value in the database..
Comment From: antirez
@oranagra yes, recently it became common... We saw different crash reports related to reaching 2 billions of keys in the same DB. @soloestoy fixed it in dict/list, later you fixed the bulk request max size, but we are still not yet to have truly 64 bit Redis that can be bothered only by the amount of memory available.
Comment From: oranagra
speaking of which, we didn't handle increasing of PROTO_MBULK_BIG_ARG. so any response to HGETALL, or SUNION with more than 32k entries will fail, right?
Comment From: antirez
@oranagra I don't remember that part, wasn't MBULK_BIG_ARG just used for optimizing the SDS string handling and end with an object that we could "grab" without memcpy()?
Comment From: oranagra
ohh, sorry, was looking at the wrong constant.
if (!ok || ll > 1024*1024) {
addReplyError(c,"Protocol error: invalid multibulk length");
setProtocolError("invalid mbulk count",c,pos);
return C_ERR;
}
Comment From: soloestoy
You mean a command which may call incrRefCount more that 2 billion times on an unshared object?
I haven't seen that, but it's really an interesting issue, thinking about that...
Comment From: soloestoy
BTW, the refcount will never overflow because the upper limit is INT_MAX:
void incrRefCount(robj *o) {
if (o->refcount != OBJ_SHARED_REFCOUNT) o->refcount++;
}
But that may lead to memory leak, because if the refcount is INT_MAX, the object will be considered as a shared object...
Comment From: antirez
@soloestoy yes I was referring to Redis versions before 4.0 (I specified that for the exact reason that later there is special handling for shared objects). The possibility after the patch that a normal objects gets shared 2 billions of times is almost null. The problem was in 3.x that the small shared integers could surely end shared 2 billions of times, and if we see decrRefCount() asserts this will be due to overflows in some instances.
Comment From: antirez
@oranagra yes there is an hard-coded limit to 1 million items in the number of commands. It's kinda a shame as well AFAIK, because it's yet another hard limit. However there it is super sensitive to have some kind of limit because otherwise something like a malformed "*292342342...." protocol string will make Redis allocate an unbound amount of memory and crash for out of memory. What could be done is to make this configurable as we did for other constants, or to create a smarter allocation mechanism so that we allocate up to a given limit, and only continue to allocate the c->argv if more bulk pieces are actually coming, but this adds definitely some complexity.
Comment From: oranagra
@antirez, obviously any hard coded limit needs to be changed to tunable (some user surely has a use case in which he needs slightly more). If you can invest the time and remove that limit completely (and rely on tunable query-buff limit to protect from abuse), that would be much better.
Comment From: yoav-steinberg
@oranagra Can we close this because it doesn't seem relevant to recent versions of redis?