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

BNER support stubs #247

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

BNER support stubs #247

wants to merge 12 commits into from

Conversation

ringlej
Copy link
Contributor

@ringlej ringlej commented Nov 19, 2017

This PR will add stubs in for BACnet Encoding Rules to reduce merge conflicts while developing the BNER encoding

@ringlej ringlej force-pushed the feature/bner-support branch 4 times, most recently from bc863e3 to 410649e Compare November 19, 2017 03:06
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 71.147% when pulling 410649e on ringlej:feature/bner-support into da672c4 on vlm:master.

@ringlej ringlej changed the title BNER support BNER support stubs Nov 19, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 71.147% when pulling 7378fa5 on ringlej:feature/bner-support into da672c4 on vlm:master.

@ringlej ringlej force-pushed the feature/bner-support branch 3 times, most recently from 493e9fc to 25110a0 Compare November 19, 2017 04:47
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 71.147% when pulling 25110a0 on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling 25110a0 on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling dae3e5c on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling dae3e5c on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling abc770b on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling 72ed265 on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling e271d55 on ringlej:feature/bner-support into da672c4 on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.135% when pulling e7474f7 on ringlej:feature/bner-support into da672c4 on vlm:master.

@ringlej
Copy link
Contributor Author

ringlej commented Nov 21, 2017

@vlm I create a branch on my fork feature/bner-support-WIP that builds on top of this PR:
https://github.com/ringlej/asn1c/commits/feature/bner-support-WIP

Once you've reviewed and merged this PR, I'll start creating other PR based on the work on the feature/bner-support-WIP branch.

Here is a link to compare your master against my feature/bner-support-WIP:
master...ringlej:feature/bner-support-WIP

@vlm
Copy link
Owner

vlm commented Nov 21, 2017

  1. Please adjust the first license line to
Copyright (c) 2003-2017  Lev Walkin <vlm@lionet.info> and contributors.

(the contributors are properly listed in the AUTHORS file).

  1. Please use ASN_DISABLE_BNER_SUPPORT instead of ASN_ENABLE_BNER_SUPPORT, just don't enable it by default in asn1c/asn1c.c. This is a hard-ish decision, but I think it is better this way, since ti makes the code uniform.


ber_tlv_tag_t bner_tag;

if(ber_tag == -1) return -1; /* ANY */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vlm
I have this check here because an asn1 rule like this:

BACnetTimeValue ::= SEQUENCE {
        time            Time,
        value           ABSTRACT-SYNTAX.&Type  -- any primitive datatype; complex types cannot be decoded
        }

will produce code like this:

asn_TYPE_member_t asn_MBR_BACnetTimeValue_1[] = {
	{ ATF_NOFLAGS, 0, offsetof(struct BACnetTimeValue, time),
		(ASN_TAG_CLASS_APPLICATION | (11 << 2)),
		0,
		&asn_DEF_Time,
		0,
		{ 0, 0, 0 },
		0, 0, /* No default value */
		"time"
		},
	{ ATF_ANY_TYPE | ATF_NOFLAGS, 0, offsetof(struct BACnetTimeValue, value),
		-1 /* Ambiguous tag (ANY?) */,
		0,
		&asn_DEF_ANY,
		0,
		{ 0, 0, 0 },
		0, 0, /* No default value */
		"value"
		},

The line in the code above:

		-1 /* Ambiguous tag (ANY?) */,

is of type ber_tlv_tag_t, which is typedef unsigned ber_tlv_tag_t

However, this line of code causes this compilation error:

bner_support.c: In function ‘convert_ber_to_bner_tag’:
bner_support.c:320:16: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
     if(ber_tag == -1) return -1; /* ANY */
                ^

I tried changing it to typedef signed ber_tlv_tag_t, but this causes tests/tests-skeletons/check-ber_tlv_tag to fail:

$ cat tests/tests-skeletons/test-suite.log
========================================================
   asn1c 0.9.29: tests/tests-skeletons/test-suite.log
========================================================

# TOTAL: 17
# PASS:  16
# SKIP:  0
# XFAIL: 0
# FAIL:  1
# XPASS: 0
# ERROR: 0

.. contents:: :depth: 2

FAIL: check-ber_tlv_tag
=======================

../../../tests/tests-skeletons/check-ber_tlv_tag.c:103:21: runtime error: left shift of negative value -294967296
    #0 0x401003 in main (/home/jringle-admin/git/asn1c/.tmp.build/tests/tests-skeletons/check-ber_tlv_tag+0x401003)
    #1 0x2aef36a22f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
    #2 0x40115f  (/home/jringle-admin/git/asn1c/.tmp.build/tests/tests-skeletons/check-ber_tlv_tag+0x40115f)

Do you have any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that X.690 section 8.1.2 doesn't seem to specify a maximum tag value allowed, but rather that the tag value can be arbitrarily large just by adding more subsequent identifier octets with the high order bit set. As such, it seems to me that the limit on the largest tag value would be limited by the implementation that is implementing the Basic Encoding Rules. Are there users of BER out there where you could reasonably expect that they would experience hardship if the largest BER tag value was cut in half from this implementation's max value due to changing it to typedef signed ber_tlv_tag_t?

Copy link
Owner

@vlm vlm Nov 22, 2017

Choose a reason for hiding this comment

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

I do not anticipate end-user problems, but it changing signedness of ber_tlv_tag_t will surely cause me going over the code base for quite some time in search of inadvertent security issues. So do it as a separate PR to make it a bit easier.

Copy link
Contributor Author

@ringlej ringlej Nov 22, 2017

Choose a reason for hiding this comment

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

@vlm I abandoned the idea of changing it to signed and instead did 5700277 (Which is part of the latest version of this this PR)

@ringlej ringlej force-pushed the feature/bner-support branch 2 times, most recently from 1c63322 to c672a2c Compare November 21, 2017 21:22
@ringlej
Copy link
Contributor Author

ringlej commented Nov 21, 2017

@vlm I must not be doing the regenerate of the test asn1 properly. I was run ./check-parsing.sh regenerate from the tests/tests-asn1c-compiler/ directory. Is there something else I need to do to fixup the tests after the e725f21 commit?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.072% when pulling 84c009b on ringlej:feature/bner-support into 4cc779f on vlm:master.

@vlm
Copy link
Owner

vlm commented Nov 22, 2017

@brchiu I usually do it using ./check-parsing.sh regenerate and verifying result with git diff . JFYI.

@ringlej
Copy link
Contributor Author

ringlej commented Nov 22, 2017

I created a separate PR #249 for the ASN_TAG_AMBIGUOUS part of this PR

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.072% when pulling 2832076 on ringlej:feature/bner-support into 4cc779f on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 71.072% when pulling 8ff07d2 on ringlej:feature/bner-support into 4cc779f on vlm:master.

The BNER support in the asn1c compiler will only support the BNER variable
encoding rules. The bner_{en,de}code() functions will to see if the PDU
being {en,de}coded is one that requires BNER fixed encoding rules (these
are "BACnet.*PDU". If it is, then it will call bner_fixed_{en,de}coder()
These two functions are declared as weak functions that will return an
error. The intent here is that an external project will provide replacement
functions for these two functions.
@ringlej ringlej force-pushed the feature/bner-support branch 3 times, most recently from 698f856 to 194762f Compare November 29, 2017 08:39
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.006% when pulling 194762f on ringlej:feature/bner-support into 4cc779f on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.006% when pulling 7c6c02f on ringlej:feature/bner-support into 4cc779f on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.006% when pulling 06e4c85 on ringlej:feature/bner-support into 4cc779f on vlm:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.006% when pulling 2788662 on ringlej:feature/bner-support into 4cc779f on vlm:master.

This will allow a bacnet library to override this function and be able to
pretty print the BNER primitives:
  Double
  CharacterString
  Date
  Time
  BACnetObjectIdentifier
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 71.031% when pulling aa62608 on ringlej:feature/bner-support into 4cc779f on vlm:master.

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.

4 participants