Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of sprintf() from HTSlib source #1594

Merged
merged 1 commit into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions cram/cram_index.c
Original file line number Diff line number Diff line change
Expand Up @@ -656,9 +656,10 @@ static int cram_index_build_multiref(cram_fd *fd,
}

if (ref != -2) {
sprintf(buf, "%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
ref, ref_start, ref_end - ref_start + 1,
(int64_t)cpos, landmark, sz);
snprintf(buf, sizeof(buf),
"%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
ref, ref_start, ref_end - ref_start + 1,
(int64_t)cpos, landmark, sz);
if (bgzf_write(fp, buf, strlen(buf)) < 0)
return -4;
}
Expand All @@ -669,9 +670,10 @@ static int cram_index_build_multiref(cram_fd *fd,
}

if (ref != -2) {
sprintf(buf, "%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
ref, ref_start, ref_end - ref_start + 1,
(int64_t)cpos, landmark, sz);
snprintf(buf, sizeof(buf),
"%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
ref, ref_start, ref_end - ref_start + 1,
(int64_t)cpos, landmark, sz);
if (bgzf_write(fp, buf, strlen(buf)) < 0)
return -4;
}
Expand Down Expand Up @@ -701,9 +703,10 @@ int cram_index_slice(cram_fd *fd,
if (s->hdr->ref_seq_id == -2) {
ret = cram_index_build_multiref(fd, c, s, fp, cpos, spos, sz);
} else {
sprintf(buf, "%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
s->hdr->ref_seq_id, s->hdr->ref_seq_start,
s->hdr->ref_seq_span, (int64_t)cpos, (int)spos, (int)sz);
snprintf(buf, sizeof(buf),
"%d\t%"PRId64"\t%"PRId64"\t%"PRId64"\t%d\t%d\n",
s->hdr->ref_seq_id, s->hdr->ref_seq_start,
s->hdr->ref_seq_span, (int64_t)cpos, (int)spos, (int)sz);
ret = (bgzf_write(fp, buf, strlen(buf)) >= 0)? 0 : -4;
}

Expand Down
6 changes: 3 additions & 3 deletions cram/cram_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -2531,7 +2531,7 @@ static refs_t *refs_load_fai(refs_t *r_orig, const char *fn, int is_err) {
/* Only the reference file provided. Get the index file name from it */
if (!(r->fn = string_dup(r->pool, fn)))
goto err;
sprintf(fai_fn, "%.*s.fai", PATH_MAX-5, fn);
snprintf(fai_fn, PATH_MAX, "%.*s.fai", PATH_MAX-5, fn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably just become %s.fai now with a size limit already enforced.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends if you want to keep the .fai on the end...

As this involves PATH_MAX which may be removed later, I'm inclined to leave this as-is for now.

Copy link
Contributor

@jkbonfield jkbonfield Mar 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I guess it's better if it truncates the pathname leaving .fai, as it prevents something catastrophic such as overwriting the main file.

(Although this looks like it's only loading, so it's just a case of preventing it from accidentally reading the wrong file on truncation)

}
}

Expand Down Expand Up @@ -4816,7 +4816,7 @@ static void full_path(char *out, char *in) {
strncpy(out, in, PATH_MAX-1);
out[PATH_MAX-1] = 0;
} else {
int len;
size_t len;

// unable to get dir or out+in is too long
if (!getcwd(out, PATH_MAX) ||
Expand All @@ -4826,7 +4826,7 @@ static void full_path(char *out, char *in) {
return;
}

sprintf(out+len, "/%.*s", PATH_MAX - 2 - len, in);
snprintf(out+len, PATH_MAX - len, "/%s", in);

// FIXME: cope with `pwd`/../../../foo.fa ?
}
Expand Down
2 changes: 1 addition & 1 deletion cram/open_trace_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ static char *expand_path(const char *file, char *dirname, int max_s_digits) {

/* Special case for "./" or absolute filenames */
if (*file == '/' || (len==1 && *dirname == '.')) {
sprintf(path, "%s", file);
memcpy(path, file, lenf + 1);
} else {
/* Handle %[0-9]*s expansions, if required */
char *path_end = path;
Expand Down
40 changes: 18 additions & 22 deletions hfile_s3.c
Original file line number Diff line number Diff line change
Expand Up @@ -451,12 +451,12 @@ static int auth_header_callback(void *ctx, char ***hdrs) {

/* like a escape path but for query strings '=' and '&' are untouched */
static char *escape_query(const char *qs) {
size_t i, j = 0, length;
size_t i, j = 0, length, alloced;
char *escaped;

length = strlen(qs);

if ((escaped = malloc(length * 3 + 1)) == NULL) {
alloced = length * 3 + 1;
if ((escaped = malloc(alloced)) == NULL) {
return NULL;
}

Expand All @@ -467,29 +467,25 @@ static char *escape_query(const char *qs) {
c == '_' || c == '-' || c == '~' || c == '.' || c == '/' || c == '=' || c == '&') {
escaped[j++] = c;
} else {
sprintf(escaped + j, "%%%02X", c);
snprintf(escaped + j, alloced - j, "%%%02X", c);
j += 3;
}
}

if (i != length) {
// in the case of a '?' copy the rest of the qs across unchanged
strcpy(escaped + j, qs + i);
} else {
escaped[j] = '\0';
}
Comment on lines -475 to -480
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this while looking at it a few days ago. It's an incorrect copy from escape_path, irrelevant here because it doesn't have the if (c == '?') break clause.

However it should probably get a mention in the commit message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now mentioned.

escaped[j] = '\0';

return escaped;
}


static char *escape_path(const char *path) {
size_t i, j = 0, length;
size_t i, j = 0, length, alloced;
char *escaped;

length = strlen(path);
alloced = length * 3 + 1;

if ((escaped = malloc(length * 3 + 1)) == NULL) {
if ((escaped = malloc(alloced)) == NULL) {
return NULL;
}

Expand All @@ -502,7 +498,7 @@ static char *escape_path(const char *path) {
c == '_' || c == '-' || c == '~' || c == '.' || c == '/') {
escaped[j++] = c;
} else {
sprintf(escaped + j, "%%%02X", c);
snprintf(escaped + j, alloced - j, "%%%02X", c);
j += 3;
}
}
Expand Down Expand Up @@ -842,14 +838,14 @@ AWS S3 sig version 4 writing code

****************************************************************/

static void hash_string(char *in, size_t length, char *out) {
static void hash_string(char *in, size_t length, char *out, size_t out_len) {
unsigned char hashed[SHA256_DIGEST_BUFSIZE];
int i, j;

s3_sha256((const unsigned char *)in, length, hashed);

for (i = 0, j = 0; i < SHA256_DIGEST_BUFSIZE; i++, j+= 2) {
sprintf(out + j, "%02x", hashed[i]);
snprintf(out + j, out_len - j, "%02x", hashed[i]);
}
}

Expand All @@ -866,7 +862,7 @@ static void ksfree(kstring_t *s) {
}


static int make_signature(s3_auth_data *ad, kstring_t *string_to_sign, char *signature_string) {
static int make_signature(s3_auth_data *ad, kstring_t *string_to_sign, char *signature_string, size_t sig_string_len) {
unsigned char date_key[SHA256_DIGEST_BUFSIZE];
unsigned char date_region_key[SHA256_DIGEST_BUFSIZE];
unsigned char date_region_service_key[SHA256_DIGEST_BUFSIZE];
Expand All @@ -893,7 +889,7 @@ static int make_signature(s3_auth_data *ad, kstring_t *string_to_sign, char *sig
s3_sign_sha256(signing_key, len, (const unsigned char *)string_to_sign->s, string_to_sign->l, signature, &len);

for (i = 0, j = 0; i < len; i++, j+= 2) {
sprintf(signature_string + j, "%02x", signature[i]);
snprintf(signature_string + j, sig_string_len - j, "%02x", signature[i]);
}

ksfree(&secret_access_key);
Expand Down Expand Up @@ -945,7 +941,7 @@ static int make_authorisation(s3_auth_data *ad, char *http_request, char *conten
goto cleanup;
}

hash_string(canonical_request.s, canonical_request.l, cr_hash);
hash_string(canonical_request.s, canonical_request.l, cr_hash, sizeof(cr_hash));

ksprintf(&scope, "%s/%s/s3/aws4_request", ad->date_short, ad->region.s);

Expand All @@ -959,7 +955,7 @@ static int make_authorisation(s3_auth_data *ad, char *http_request, char *conten
goto cleanup;
}

if (make_signature(ad, &string_to_sign, signature_string)) {
if (make_signature(ad, &string_to_sign, signature_string, sizeof(signature_string))) {
goto cleanup;
}

Expand Down Expand Up @@ -1094,10 +1090,10 @@ static int write_authorisation_callback(void *auth, char *request, kstring_t *co
}

if (content) {
hash_string(content->s, content->l, content_hash);
hash_string(content->s, content->l, content_hash, sizeof(content_hash));
} else {
// empty hash
hash_string("", 0, content_hash);
hash_string("", 0, content_hash, sizeof(content_hash));
}

ad->canonical_query_string.l = 0;
Expand Down Expand Up @@ -1166,7 +1162,7 @@ static int v4_auth_header_callback(void *ctx, char ***hdrs) {
return copy_auth_headers(ad, hdrs);
}

hash_string("", 0, content_hash); // empty hash
hash_string("", 0, content_hash, sizeof(content_hash)); // empty hash

ad->canonical_query_string.l = 0;

Expand Down
2 changes: 1 addition & 1 deletion kstring.c
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ int kputd(double d, kstring_t *s) {
if (ks_resize(s, s->l + 50) < 0)
return EOF;
// We let stdio handle the exponent cases
int s2 = sprintf(s->s + s->l, "%g", d);
int s2 = snprintf(s->s + s->l, s->m - s->l, "%g", d);
len += s2;
s->l += s2;
return len;
Expand Down
2 changes: 1 addition & 1 deletion plugin.c
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ const char *hts_plugin_path(void) {
}

static char s_path[1024];
sprintf(s_path, "%.1023s", ks.s ? ks.s : "");
snprintf(s_path, sizeof(s_path), "%s", ks.s ? ks.s : "");
free(ks.s);

return s_path;
Expand Down
20 changes: 11 additions & 9 deletions sam.c
Original file line number Diff line number Diff line change
Expand Up @@ -5392,20 +5392,22 @@ int bam_plp_insertion_mod(const bam_pileup1_t *p,
for (j = 0; j < nm; j++) {
char qual[20];
if (mod[j].qual >= 0)
sprintf(qual, "%d", mod[j].qual);
snprintf(qual, sizeof(qual), "%d", mod[j].qual);
else
*qual=0;
if (mod[j].modified_base < 0)
// ChEBI
indel += sprintf(&ins->s[indel], "%c(%d)%s",
"+-"[mod[j].strand],
-mod[j].modified_base,
qual);
indel += snprintf(&ins->s[indel], ins->m - indel,
"%c(%d)%s",
"+-"[mod[j].strand],
-mod[j].modified_base,
qual);
else
indel += sprintf(&ins->s[indel], "%c%c%s",
"+-"[mod[j].strand],
mod[j].modified_base,
qual);
indel += snprintf(&ins->s[indel], ins->m - indel,
"%c%c%s",
"+-"[mod[j].strand],
mod[j].modified_base,
qual);
}
ins->s[indel++] = ']';
ins->l += indel - o_indel; // grow by amount we used
Expand Down
2 changes: 1 addition & 1 deletion test/hfile.c
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ int main(void)
original = slurp("vcf.c");
for (i = 1; i <= 6; i++) {
char *text;
sprintf(buffer, "test/hfile%d.tmp", i);
snprintf(buffer, sizeof(buffer), "test/hfile%d.tmp", i);
text = slurp(buffer);
if (strcmp(original, text) != 0) {
fprintf(stderr, "%s differs from vcf.c\n", buffer);
Expand Down
2 changes: 1 addition & 1 deletion test/sam.c
Original file line number Diff line number Diff line change
Expand Up @@ -1504,7 +1504,7 @@ static void faidx1(const char *filename)

fin = fopen(filename, "rb");
if (fin == NULL) fail("can't open %s", filename);
sprintf(tmpfilename, "%s.tmp", filename);
snprintf(tmpfilename, sizeof(tmpfilename), "%s.tmp", filename);
fout = fopen(tmpfilename, "wb");
if (fout == NULL) fail("can't create temporary %s", tmpfilename);
while (fgets(line, sizeof line, fin)) {
Expand Down
22 changes: 11 additions & 11 deletions test/test-regidx.c
Original file line number Diff line number Diff line change
Expand Up @@ -304,20 +304,20 @@ void test_explicit(char *tgt, char *qry, char *exp)
regidx_destroy(idx);
}

void create_line_bed(char *line, char *chr, int start, int end)
void create_line_bed(char *line, size_t size, char *chr, int start, int end)
{
sprintf(line,"%s\t%d\t%d\n",chr,start-1,end);
snprintf(line,size,"%s\t%d\t%d\n",chr,start-1,end);
}
void create_line_tab(char *line, char *chr, int start, int end)
void create_line_tab(char *line, size_t size, char *chr, int start, int end)
{
sprintf(line,"%s\t%d\t%d\n",chr,start,end);
snprintf(line,size,"%s\t%d\t%d\n",chr,start,end);
}
void create_line_reg(char *line, char *chr, int start, int end)
void create_line_reg(char *line, size_t size, char *chr, int start, int end)
{
sprintf(line,"%s:%d-%d\n",chr,start,end);
snprintf(line,size,"%s:%d-%d\n",chr,start,end);
}

typedef void (*set_line_f)(char *line, char *chr, int start, int end);
typedef void (*set_line_f)(char *line, size_t size, char *chr, int start, int end);

void test(set_line_f set_line, regidx_parse_f parse)
{
Expand All @@ -329,17 +329,17 @@ void test(set_line_f set_line, regidx_parse_f parse)
for (i=1; i<n; i++)
{
start = end = 10*i;
set_line(line,chr,start,end);
set_line(line,sizeof(line),chr,start,end);
debug("insert: %s", line);
if ( regidx_insert(idx,line)!=0 ) error("insert failed: %s\n", line);

start = end = 10*i + 1;
set_line(line,chr,start,end);
set_line(line,sizeof(line),chr,start,end);
debug("insert: %s", line);
if ( regidx_insert(idx,line)!=0 ) error("insert failed: %s\n", line);

start = 20000*i; end = start + 2000;
set_line(line,chr,start,end);
set_line(line,sizeof(line),chr,start,end);
debug("insert: %s", line);
if ( regidx_insert(idx,line)!=0 ) error("insert failed: %s\n", line);
}
Expand Down Expand Up @@ -396,7 +396,7 @@ void test(set_line_f set_line, regidx_parse_f parse)

// fully contained interval, one hit
start = 20000*i - 5000; end = 20000*i + 3000;
set_line(line,chr,start,end);
set_line(line,sizeof(line),chr,start,end);
if ( !regidx_overlap(idx,chr,start-1,end-1,itr) ) error("query failed, there should be a hit: %s:%d-%d\n",chr,start,end);
debug("ok: overlap(s) found for %s:%d-%d\n",chr,start,end);
nhit = 0;
Expand Down
Loading