Make all inputs constant by rhpvorderman · Pull Request #152 · samtools/htscodecs

rhpvorderman · 2026-07-06T18:09:47Z

This makes all inputs constant, because input data should not be changed by encoding/decoding functions.

However I found two things in the code that make this not yet possible:

Line 1500 in tokenise_name3.c

blk[i] = '\0';

This overwrites the input with zeroes.

Line 1807 in rANS_static4x16pr.c
meta_free = meta = rans_dec_func(do_simd, 0)(in+sz, in_size-sz, NULL, u_meta_size);

Meta is equal to the input pointer + something. Meta_free is a mutable buffer. Here both get equated for some reason? I don't fully understand what is happening here.

jkbonfield · 2026-07-07T08:16:23Z

The tokenise_name one could probably be amended. It already passes in the length to encode_name so it doesn't need a nul terminated string. It does cache a copy of it as last_name, but it could also have a last name length field to ease comparisons. So I think it's viable to fix this.

The rans decode function should be easily fixable.
If the meta data in the stream is uncompressed, then meta is just a pointer to it. If it's compressed, then we uncompress it and meta is now a newly allocated buffer, which is also copied into meta_free as an indication that the function needs to free it at the end, unlike the uncompressed case. The logic is fine, but we can reverse it in the way we think about it.

We uncompress the data and store the new buffer into the meta_free pointer, and take a const copy of that in meta.

-            meta_free = meta = rans_dec_func(do_simd, 0)(in+sz, in_size-sz, NULL, u_meta_size);
+            meta = meta_free = rans_dec_func(do_simd, 0)(in+sz, in_size-sz, NULL, u_meta_size);

So meta_free is non-const (necessary as free isn't const) and then meta is a const copy of that pointer which is compatible with the uncompressed meta-data variant.

jkbonfield · 2026-07-07T08:42:28Z

This looks to fix the name tokeniser, at a cost of a tiny amount of memory to store the read name length. (It could in theory be less due to a 255 char limit in the specification, but I doubt it'd make any difference with structure padding taking up the other bytes)

diff --git a/htscodecs/tokenise_name3.c b/htscodecs/tokenise_name3.c
index 866cc57..c1dcdbb 100644
--- a/htscodecs/tokenise_name3.c
+++ b/htscodecs/tokenise_name3.c
@@ -135,7 +135,8 @@ typedef struct {
 } last_context_tok;
 
 typedef struct {
-    char *last_name;
+    const char *last_name;
+    int last_name_len;
     int last_ntok;
     last_context_tok *last; // [last_ntok]
 } last_context;
@@ -409,7 +410,7 @@ static int decode_token_int1(name_context *ctx, int ntok,
 // Maybe XOR with previous string as context?
 // This permits partial match to be encoded efficiently.
 static int encode_token_alpha(name_context *ctx, int ntok,
-                              char *str, int len) {
+                              const char *str, int len) {
     int id = (ntok<<4) | N_ALPHA;
 
     if (encode_token_type(ctx, ntok, N_ALPHA) < 0)  return -1;
@@ -587,7 +588,7 @@ void dump_trie(trie_t *t, int depth) {
 #endif
 
 static
-int search_trie(name_context *ctx, char *data, size_t len, int n, int *exact, int *is_fixed, int *fixed_len) {
+int search_trie(name_context *ctx, const char *data, size_t len, int n, int *exact, int *is_fixed, int *fixed_len) {
     size_t i;
     trie_t *t;
     int from = -1, p3 = -1;
@@ -598,7 +599,7 @@ int search_trie(name_context *ctx, char *data, size_t len, int n, int *exact, in
     // Horrid hack for the encoder only.
     // We optimise per known name format here.
     int prefix_len;
-    char *d = *data == '@' ? data+1 : data;
+    const char *d = *data == '@' ? data+1 : data;
     int l   = *data == '@' ? len-1  : len;
     int f = (*data == '>') ? 1 : 0;
     if (l > 70 && d[f+0] == 'm' && d[7] == '_' && d[f+14] == '_' && d[f+61] == '/') {
@@ -692,7 +693,7 @@ int search_trie(name_context *ctx, char *data, size_t len, int n, int *exact, in
  * Returns 0 on success;
  *        -1 on failure.
  */
-static int encode_name(name_context *ctx, char *name, int len, int mode) {
+static int encode_name(name_context *ctx, const char *name, int len, int mode) {
     int i, is_fixed, fixed_len;
 
     int exact;
@@ -703,14 +704,17 @@ static int encode_name(name_context *ctx, char *name, int len, int mode) {
     //cnum = cnum & (MAX_NAMES-1);
     //if (pnum == cnum) {pnum = cnum ? cnum-1 : 0;}
 #ifdef ENC_DEBUG
-    fprintf(stderr, "%d: pnum=%d (%d), exact=%d\n%s\n%s\n",
-            ctx->counter, pnum, cnum-pnum, exact, ctx->lc[pnum].last_name, name);
+    fprintf(stderr, "%d: pnum=%d (%d), exact=%d\n%.*s\n%.*s\n",
+            ctx->counter, pnum, cnum-pnum, exact,
+            ctx->lc[pnum].last_name_len, ctx->lc[pnum].last_name,
+            len, name);
 #endif
 
     // Return DUP or DIFF switch, plus the distance.
-    if (exact && len == strlen(ctx->lc[pnum].last_name)) {
+    if (exact && len == ctx->lc[pnum].last_name_len) {
         encode_token_dup(ctx, cnum-pnum);
         ctx->lc[cnum].last_name = name;
+        ctx->lc[cnum].last_name_len = len;
         ctx->lc[cnum].last_ntok = ctx->lc[pnum].last_ntok;
         int nc = ctx->lc[cnum].last_ntok ? ctx->lc[cnum].last_ntok : MAX_TOKENS;
         ctx->lc[cnum].last = malloc(nc * sizeof(*ctx->lc[cnum].last));
@@ -1003,6 +1007,7 @@ static int encode_name(name_context *ctx, char *name, int len, int mode) {
     //printf("Encoded %.*s with %d tokens\n", len, name, ntok);
     
     ctx->lc[cnum].last_name = name;
+    ctx->lc[cnum].last_name_len = len;
     ctx->lc[cnum].last_ntok = ntok;
     last_context_tok *shrunk = realloc(ctx->lc[cnum].last,
                                        (ntok+1) * sizeof(*ctx->lc[cnum].last));
@@ -1472,7 +1477,6 @@ uint8_t *tok3_encode_names(const char *blk, int len, int level, int use_arith,
         if (i >= len)
             break;
 
-        //blk[i] = '\0';
         last_start = i+1;
         if (build_trie(ctx, &blk[j], i-j, ctr++) < 0) {
             free_context(ctx);
@@ -1497,7 +1501,6 @@ uint8_t *tok3_encode_names(const char *blk, int len, int level, int use_arith,
             return NULL;
         }
 
-        blk[i] = '\0';
         // try both 0 and 1 and pick best?
         if (encode_name(ctx, &blk[j], i-j, 1) < 0) {
             free_context(ctx);

rhpvorderman · 2026-07-07T08:47:09Z

Thanks! I will implement that. I was just in the process of doing so.

rhpvorderman · 2026-07-07T09:30:00Z

Right, this might look great and all, but I get errors when I run
./configure CFLAGS="-Wall -Werror -fsanitize=address,undefined -Og -g"
make check

Compiling is fine but the name tokenizer does not finish. That needs to be fixed before this can be merged.

rhpvorderman · 2026-07-07T09:36:16Z

Everything fixed and ready for review!

jkbonfield · 2026-07-07T10:35:26Z

@@ -1045,7 +1045,7 @@ static int decode_name(name_context *ctx, char *name, int name_len) {
            return -1;

        if (strlen(ctx->lc[pnum].last_name) +1 >= name_len) return -1;


It looks like I missed a last_name fix in my change!

I was wondering how memcpy to memmove fixes anything, as the name and last_name should never overlap. How has changing from nul-terminated strings to str+length made them overlap? The answer is when I forget to check the length and still rely on strlen! Oops.

Ah wait... this is the decoder, which I don't think we changed for the blk[i] = '\0' bit.

I need to ponder it more I think.

Ok I see the cause of this now. The original code in the decoder was this:

if (t0 == N_DUP) { if (pnum == cnum) return -1; if (strlen(ctx->lc[pnum].last_name) +1 >= name_len) return -1; strcpy(name, ctx->lc[pnum].last_name); // FIXME: optimise this ctx->lc[cnum].last_name = name; ctx->lc[cnum].last_ntok = ctx->lc[pnum].last_ntok;

It may look funky and like it'll have a problem with removing the blk[i] = '\0' line, but that was in the encoder. Hence I didn't change it and didn't need to either. The decoder is already producing a nul-terminated output array. This isn't ideal, but this is a specialised codec that is returning an array of read names, and the terminating symbol (nul or newline) isn't part of the data stream. (Hence we have a horrible tr in the comparison code to check it round trips OK.)

So, the names (including ctx->lc[pnum].last_name which is just a pointer into the output buffer decoded so far if I recall correctly) are indeed still nul-terminated here, and strcpy works fine. That said, memcpy would be faster if we knew the length, and thanks to expanding the context struct we do now have a slot for storing the length. So it is worth improving this and my initial proposal wasn't optimal I think.

Your initial change was to this:

if (t0 == N_DUP) { if (pnum == cnum) return -1; if (strlen(ctx->lc[pnum].last_name) +1 >= name_len) return -1; memcpy(name, ctx->lc[pnum].last_name, name_len); // FIXME: optimise this ctx->lc[cnum].last_name = name; ctx->lc[cnum].last_name_len = name_len; ctx->lc[cnum].last_ntok = ctx->lc[pnum].last_ntok;

Thhis didn't work as you're copying name_len and not strlen(ctx->lc[pnum].last_name) +1 bytes worth out of last_name.

Here I do have to apologise though! name_len in this function is the size of the name array:

static int decode_name(name_context *ctx, char *name, int name_len) {

It's not the length of an individual name. I should probably have called it something different. Sorry. You can see this in the calling code:

size_t out_sz = 0; while ((ret = decode_name(ctx, (char *)out+out_sz, ulen)) > 0) { out_sz += ret; ulen -= ret; }

Hence we're copying vast swathes of data with that memcpy. Some 40kb worth on my test. Memmove solves it, but it's the wrong fix and hides a new O(N^2) complexity loop.
We could cache the strlen return value and use that, but we can already do that in last_name_len now anyway. Ie:

diff --git a/htscodecs/tokenise_name3.c b/htscodecs/tokenise_name3.c index a6fd9e7..5ccd035 100644 --- a/htscodecs/tokenise_name3.c +++ b/htscodecs/tokenise_name3.c @@ -1044,11 +1044,15 @@ static int decode_name(name_context *ctx, char *name, int name_len) { if (pnum == cnum) return -1; - if (strlen(ctx->lc[pnum].last_name) +1 >= name_len) return -1; - memmove(name, ctx->lc[pnum].last_name, name_len); + int last_name_len = ctx->lc[pnum].last_name_len; + if (last_name_len >= name_len) return -1; + memcpy(name, ctx->lc[pnum].last_name, last_name_len); // FIXME: optimise this ctx->lc[cnum].last_name = name; - ctx->lc[cnum].last_name_len = name_len; + ctx->lc[cnum].last_name_len = last_name_len; ctx->lc[cnum].last_ntok = ctx->lc[pnum].last_ntok; int nc = ctx->lc[cnum].last_ntok ? ctx->lc[cnum].last_ntok : MAX_TOKENS; @@ -1194,7 +1198,7 @@ static int decode_name(name_context *ctx, char *name, int name_len) { name[len++] = 0; ctx->lc[cnum].last[ntok].token_type = N_END; ctx->lc[cnum].last_name = name; - ctx->lc[cnum].last_name_len = name_len; + ctx->lc[cnum].last_name_len = len; ctx->lc[cnum].last_ntok = ntok; last_context_tok *shrunk

This caches the actual length of the single name returned.

Again, sorry about the awful reuse of name_len for two totally different use cases in the encode function vs the decode function! We should probably call it something else like capacity.

number_of_names comes to mind.

I can work on this later. I believe I turned on changes by owners in case you just want to get it over with.

I was indeed surprised by the adress sanitizers warning of overlapping lengths. But given your explanation that makes sense now. What is more surprising is the fact that there is a bug, and still all the tests run fine. With C I always get a little paranoid about what kind of memory I did change in that instance. This could cause very interesting intermittent bugs down the road. I think the memmove should be a memcpy again in any case so address sanitizer will properly catch that it is fixed.

number_of_names comes to mind.

It's not the number of names, but number of bytes used to store the names. name_capacity perhaps? It's basically there to avoid buffer overruns incase the encoded data stream is lying about the unpacked size.

name_buffer_size?

Done. Switched up to memcpy again. This time no errors from address sanitizer. And I trust address sanitizer. Such a great tool to work with.
I think I put all the right lens in the right places.

Also I have to apologize for the amount of time-investment required for you. I still feel guilty about the last time where my idea turned out to be quite a waste of time.

Don't beat yourself up unnecessarily. You're good to work with and even your previous idea that you referred to did highlight flaws in the original code which wouldn't have been fixed had you not created that PR. Maybe the fix itself wasn't correct, but the code still improved due to your time and effort!

Software engineering is rarely a smooth path in the direction you assume it's going. :-)

Thanks for this. It may be another week or two before I get back to checking it though (depending on my time tomorrow). It's looking promising though.

Thanks for the kind words. That means a lot.

Please take your time. I can simply build the python bindings on this commit for the time being. I made the python bindings just because I want to mess around with these very good codecs. I want to package them in a way that is more "mess-a-roundable" than a C library. In other words, pip install htscodecs and then import htscodecs, done instead of configuring compile dependencies etc. Python also provides a nice REPL so you can interactively mess around, maybe use iPython notebooks etc.

I am going to abuse the name tokeniser to reproducibly tackle float representations. Converting back to binary and then back again leads to a loss of the original representation. I was thinking about ways to tackle that and realised the name tokeniser actually tackles that particular problem. So I wonder how it performs against generic compression algorithms.

Make all inputs constant

8fc7cd9

rhpvorderman added 2 commits July 7, 2026 10:57

Fix tokenise names and rans decode function for use with const inputs

653f7f4

Fix ARM const pointers

567fa9b

Prevent overlapping memory copy by using memmove

d096c90

rhpvorderman added 2 commits July 7, 2026 11:48

Add const to API documentation

63acb7e

Add changelog item for const change

2e20f18

rhpvorderman force-pushed the const branch from 1e3f629 to 2e20f18 Compare July 7, 2026 09:56

jkbonfield reviewed Jul 7, 2026

View reviewed changes

rhpvorderman added 2 commits July 8, 2026 10:13

Rename name_length to name_buffer_size, properly assign last_name_len

96add4b

Fix unnecessary lookup

d88efa6

		@@ -1045,7 +1045,7 @@ static int decode_name(name_context ctx, char name, int name_len) {
		return -1;

		if (strlen(ctx->lc[pnum].last_name) +1 >= name_len) return -1;

Uh oh!

Conversation

rhpvorderman commented Jul 6, 2026

Uh oh!

jkbonfield commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkbonfield commented Jul 7, 2026

Uh oh!

rhpvorderman commented Jul 7, 2026

Uh oh!

rhpvorderman commented Jul 7, 2026

Uh oh!

rhpvorderman commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkbonfield Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jkbonfield commented Jul 7, 2026 •

edited

Loading

jkbonfield Jul 7, 2026 •

edited

Loading