View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006501 | unreal | ircd | public | 2025-03-29 06:33 | 2025-03-30 07:20 |
| Reporter | bsstephan | Assigned To | |||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | new | Resolution | open | ||
| Product Version | 6.1.9 | ||||
| Summary | 0006501: CHATHISTORY BETWEEN treats both directions the same way | ||||
| Description | I have been working on an implementation of a client requesting CHATHISTORY and the use case I'm focused on is getting slices via CHATHISTORY BETWEEN. Testing on my network has led me to the conclusion that CHATHISTORY BETWEEN behavior does not match the spec, and I believe I understand the reason why in the code. Take for instance this situation: assume the history is 100 messages long, msgids 1 (oldest) through 100 (newest). If I request BETWEEN msgid=1 msgid=100 5 (never mind for the moment how I know 100 is the newest, in practice I use a timestamp). I should get 2,3,4,5,6 as I started at the first message selector and went forward in time. If I request BETWEEN msgid=100 msgid=1 5, I should get 99,98,97,96,95 (from first selector but backwards in time). (I asked in #ircv3 and this was agreed to be the intended behavior --- "With respect to the limit, the returned messages MUST be counted _starting from and excluding the first message selector_, while finishing on and excluding the second. This may be forwards or backwards in time.", emphasis mine.) However, on my ircd (6.1.9), in my testing I get the same set, 2,3,4,5,6, in either request. Digging into the code I think I understand why. hbm_return_between_figure_out_direction is a bit hard to understand, but assuming it works correctly, the implementation of hbm_return_between is incorrect for the backwards case. How the code seems to work: * BETWEEN msgid=1 msgid=100 * direction == forwards * return hbm_return_after() w/ timestamp_a null, timestamp_b null, msgid_a = 1, msgid_b = 100, limit = 5 * BETWEEN msgid=100 msgid=1 * direction == backwards * create new filter * swap timestamp_a and timestamp_b (null and null, no effect) * swap msgid_a and msgid_b (parameter msgid=1 becomes msgid_a, parameter msgid=100 becomes msgid_b) * return hbm_return_after() w/ timestamp_a null, timestamp_b null, msgid_a = 1, msgid_b = 100, limit = 5 Hence the identical results for both. I may still dig deeper to understand if there's a quick fix in how the library methods work, but I believe my reading of the defect is correct at least. | ||||
| Steps To Reproduce | In a channel with a history bigger than what you are requesting, try both: CHATHISTORY BETWEEN #foo msgid=foo msgid=bar 5 CHATHISTORY BETWEEN #foo msgid=bar msgid=foo 5 ...and they will return the same slice of history despite the spec specifying that the first parameter is where the history should start. You can also do a slightly more organic CHATHISTORY BETWEEN #foo msgid=foo timestamp=now 5 CHATHISTORY BETWEEN #foo timestamp=now msgid=foo 5 ...since that is the realistic case, semantically, "show me 5 messages after foo, which I already saw" vs. "show me the 5 latest messages, but stop at foo because I already read it". And of course note that if the limit is >= the history, you can't tell the difference because you get the same history either way. This defect only manifests if you are getting a subset of the history. | ||||
| Tags | No tags attached. | ||||
| 3rd party modules | |||||
|
|
hbm_return_after and hbm_return_before make similar "Check if we need to stop"checks to stop before the limit, do we even need the temporary filter, and instead just call hbm_return_before? |
|
|
Local testing suggests that yes, this is sufficient: diff --git a/src/modules/history_backend_mem.c b/src/modules/history_backend_mem.c index cad23e93d..b11b8fa79 100644 --- a/src/modules/history_backend_mem.c +++ b/src/modules/history_backend_mem.c @@ -1139,17 +1139,7 @@ static int hbm_return_between(HistoryResult *r, HistoryLogObject *h, HistoryFilt } else if (direction == 0) { - /* Create a temporary filter, swapping directions */ - char *x, *y; - HistoryFilter f; - memset(&f, 0, sizeof(f)); - f.cmd = HFC_BEFORE; - f.limit = filter->limit; - f.timestamp_a = filter->timestamp_b; - f.timestamp_b = filter->timestamp_a; - f.msgid_a = filter->msgid_b; - f.msgid_b = filter->msgid_a; - return hbm_return_after(r, h, &f); + return hbm_return_before(r, h, filter); } /* else direction is -1 which means not found / invalid */ |
