Skip to content

misuse of compare_testvector#188

Merged
sjaeckel merged 1 commit into
libtom:developfrom
fperrad:20170405_lint
Apr 6, 2017
Merged

misuse of compare_testvector#188
sjaeckel merged 1 commit into
libtom:developfrom
fperrad:20170405_lint

Conversation

@fperrad
Copy link
Copy Markdown
Contributor

@fperrad fperrad commented Apr 5, 2017

compare_testvector has two different definitions see https://github.com/libtom/libtomcrypt/blob/develop/src/headers/tomcrypt_misc.h#L102-L109
When the macro definition is used, the comparison compare_testvector() != 0 is wrong.
When the function definition is used, the comparison is useless.
So, this commit removes the comparison.

@sjaeckel
Copy link
Copy Markdown
Member

sjaeckel commented Apr 5, 2017

very true

@sjaeckel
Copy link
Copy Markdown
Member

sjaeckel commented Apr 5, 2017

I just thought would it make sense to add this patch instead?

diff --git a/src/headers/tomcrypt_misc.h b/src/headers/tomcrypt_misc.h
index 91f87c5..76f4f6b 100644
--- a/src/headers/tomcrypt_misc.h
+++ b/src/headers/tomcrypt_misc.h
@@ -105,7 +105,7 @@ void print_hex(const char* what, const void* v, const unsigned long l);
 int compare_testvector(const void* is, const unsigned long is_len, const void* should, const unsigned long should_len, const
 #else
 #define compare_testvector(is, is_len, should, should_len, what, which) \
-   (((is_len) != (should_len)) || (XMEMCMP((is), (should), (is_len)) != 0))
+   ((((is_len) != (should_len)) || (XMEMCMP((is), (should), (is_len)) != 0)) ? 1 : 0)
 #endif
 
 /* $Source$ */

@karel-m
Copy link
Copy Markdown
Member

karel-m commented Apr 5, 2017

The macro compare_testvector should be fixed as proposed by @sjaeckel anyway.

Using if (compare_testvector(..)) vs. if(compare_testvector(..) != 0) is just a matter of taste. Its nature is similar to XMEMCMP which we use in both ways if (XMEMCMP(..)) as well as if (XMEMCMP(..) != 0). I think we can leave out != 0 in these cases.

@sjaeckel sjaeckel merged commit e4331bc into libtom:develop Apr 6, 2017
@sjaeckel sjaeckel modified the milestone: v1.18.0 Jun 27, 2017
sjaeckel pushed a commit that referenced this pull request Oct 10, 2017
An ARM compiler gives me this: 

libtomcrypt\pk\asn1\der\sequence\der_decode_subject_public_key_info.c(65,4): error #188-D: enumerated type mixed with another type

Since der_decode_subject_public_key_info's parameters_type is of type 'unsigned long', an attempt to assign it to ltc_asn1_list's member 'ltc_asn1_type type' fails.

My fix solves this in a simple way by casting it at the point of assignment.

But while studying this I noticed there's no use of enum in the codebase other than a few PK-related things.  Perhaps a more appropriate solution would be to remove these enums. I mean, enums seem like an OK enough idea, but I don't know anything about the practicality of using enums in archaic C dialects like libtomcrypt conforms (thankfully!) to...
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

Successfully merging this pull request may close these issues.

3 participants