Fix PSBT marshal/unmarshal functions. - #191
Conversation
| memcpy(*cursor, src, *max); | ||
| /* From now on, max records the room we *needed* */ | ||
| *max = len - *max; | ||
| *cursor = NULL; |
There was a problem hiding this comment.
What's the purpose of doing this on failure?
There was a problem hiding this comment.
*cursor == NULL is the way you detect unmarshalling failure.
*max increasing is how you tell how many bytes short you are.
You'll see this explicitly used here:
if (cursor == NULL) {
/* Once cursor was NULL, max accumulates hm bytes we needed */
*bytes_written = len + max;
return WALLY_EINVAL;
} else {
*bytes_written = len - max;
}
|
Concept ACK |
234e885 to
e7023db
Compare
rustyrussell
changed the title
e7023db to
52d5ba8
Compare
|
This still fails travis, but I can't tell why since the logs are insufficient :( |
|
Look at tools/travis_build.sh and add your new test to it. you should have the output of it somewhere in the build output |
52d5ba8 to
3931357
Compare
OK, I fixed it a different way in the first commit: let's see how that looks? |
08a1f16 to
e9100ff
Compare
|
Nice. #197 fixes the actual travis build so hopefully all current outstanding PRs will have a green build. |
168a926 to
a874af4
Compare
achow101
left a comment
achow101
left a comment
There was a problem hiding this comment.
A few comments
- In 8484e97 "travis: don't explicitly list what tests in tools/travis_build.sh": Maybe don't @ mention people in commit messages. In Core, we have a policy that commit messages shouldn't do that as those people will get spammed when forks pick up those commits.
- In 637e4c5 "psbt: add a simple test case, by copying some of BIP 0174.":
src/test_psbt*needs to be added to.gitignore. - In e06ad79 "pullpush: add routines for safe marshal / unmarshal.": There are a few places where
maxandcursorare dereferenced without checking that they are not null.. This seems okay to me as the pushpull functions are internal and we would want to see a segfault as that indicates we are using them improperly. But maybe that's not the intention?
|
|
||
| void push_u8(unsigned char **cursor, size_t *max, uint8_t v) | ||
| { | ||
| push_bytes(cursor, max, &v, 1); |
There was a problem hiding this comment.
In e06ad79 "pullpush: add routines for safe marshal / unmarshal."
nit: sizeof(v) instead of 1
AFAICT, the below does the same thing, and avoids glslang having to tell me to add my test there. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As revealed by: make && LD_LIBRARY_PATH=`pwd`/src/.libs valgrind --leak-check=full src/.libs/test_psbt There are more, but the next patches rework that code. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Ah, I gave up reading my GH notifications long ago, so I never noticed. Good idea, thanks!
Ack, and same with limits test. (EDIT: actually, they use such gross wildcards in .gitignore that they get covered too. This is bad practice, but that's a separate issue).
Yes, they're internal only, and have specific semantics; adding checks like this would seem to add nothing but clutter. In particular, there's no infra in libwally for "crash iff in dev mode" which would allow CI to be effective in this case. If we were to return WALLY_EINVAL on internal errors, we might not notice them in CI. |
Manually checking for end of buffers is error prone: these pull & push variants are similar to those used by c-lightning. Importantly, if they fail you can still call them, meaning you can check whether it overflowed at the end of all the work. The push function also can tell you exactly how many bytes short you were. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This also means we return WALLY_EINVAL instead of corrupting the stack if they provide a witness output > 50 bytes. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
As a special bonus, document that it now returns bytes_written as the entire length, if the error was caused by bytes_len being insufficient. This allows the caller to avoid calling bytes_len in the common case, if they want to. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…ytes. This avoid (recently buggy!) duplication. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Now we pull the magic before calling count_psbt_parts(), it no longer needs to skip over it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This currently fails (badly). Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
valgrind is better, but this is simple and finds current leaks. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
a874af4 to
1cccf05
Compare
|
Rebased with fixes from @achow101 (all minor). |
|
ACK 1cccf05 |
|
@glslang @greenaddress Can this be merged? |
|
Closing this in favour of #210 which includes/supersedes it. Please re-open if something you need got missed in the merge. |
4 participants
Started with PSBT, will continue. But worth reviewing.