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

bignum_from_string: only works with hex-strings w/ multiple of 8 characters #14

Closed
DEF7 opened this issue Jan 20, 2020 · 3 comments
Closed

Comments

@DEF7
Copy link

DEF7 commented Jan 20, 2020

The problem lies with this piece of code:

tiny-bignum-c/bn.c

Lines 108 to 120 in e814d2b

int i = nbytes - (2 * WORD_SIZE); /* index into string */
int j = 0; /* index into array */
/* reading last hex-byte "MSB" from string first -> big endian */
/* MSB ~= most significant byte / block ? :) */
while (i >= 0)
{
tmp = 0;
sscanf(&str[i], SSCANF_FORMAT_STR, &tmp);
n->array[j] = tmp;
i -= (2 * WORD_SIZE); /* step WORD_SIZE hex-byte(s) back in the string. */
j += 1; /* step one element forward in the array. */
}

which appears as:

`
int i = nbytes - (2 * WORD_SIZE); /* index into string /
int j = 0; /
index into array */

/* reading last hex-byte "MSB" from string first -> big endian /
/
MSB ~= most significant byte / block ? :) /
while (i >= 0)
{
tmp = 0;
sscanf(&str[i], SSCANF_FORMAT_STR, &tmp);
n->array[j] = tmp;
i -= (2 * WORD_SIZE); /
step WORD_SIZE hex-byte(s) back in the string. /
j += 1; /
step one element forward in the array. */
}
`

If 'i' (the length of the inbound string, in characters) is not a multiple of '2 * WORD_SIZE' then it will skip over the MSB hexadecimal characters at the beginning of the string when 'i < 0' and miss the characters at the beginning of the inbound string, incorrectly setting 'n'.

I think the use of sprintf/sscanf is not ideal. Custom to/from string routines would be better.

@kokke
Copy link
Owner

kokke commented Jan 20, 2020

Hi @DEF7 and thanks for your interest in this project :)

You're correct that the to/from_string methods only works for a 8-bite chunks, but this is by design.

I only use the functions for the test-code, which I hint at in the readme: **stdio.h is only used for testing functions parsing to and from hex-strings.**. This is the reason for not implementing better to/from_string methods -- they are only used for the testing.

Custom to/from string routines would be better.

I agree. I would most likely accept a PR that improved the current situation :)

@DEF7
Copy link
Author

DEF7 commented Jan 20, 2020 via email

@kokke
Copy link
Owner

kokke commented Jan 21, 2020

Until further notice, I think it's fair to add this piece of assertion-code in bignum_from_string:

require((nbytes % (sizeof(DTYPE) * 2)) == 0, "string length must be a multiple of (sizeof(DTYPE) * 2) characters");

,

... so that you at least get a proper failure/crash with a line number etc., if you mess up:

,

test_hand_picked: bn.c:104: bignum_from_string: Assertion `(nbytes % (sizeof(uint16_t) * 2)) == 0 && "string length must be a multiple of (sizeof(DTYPE) * 2) characters"' failed.

What do you think @DEF7 ?

kokke added a commit that referenced this issue Feb 3, 2020
@kokke kokke closed this as completed Feb 3, 2020
kachsheev pushed a commit to kachsheev/tiny-bignum-c that referenced this issue Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants