Open
Bug 304875
Opened 19 years ago
Updated 2 years ago
extend original decoder functionality to handle OPTIONAL INLINES of simple templates.
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
NEW
People
(Reporter: alvolkov.bgs, Unassigned)
Details
Attachments
(1 file)
3.09 KB,
patch
|
nelson
:
review-
|
Details | Diff | Splinter Review |
Original decoder, unlike quickDER, does not support this feature that makes
tests generated with dynamic data generator fail on some templates.
See also: 280121
Reporter | ||
Comment 1•19 years ago
|
||
First, allow continue with INLINES that have OPTIONAL tag until we get a
subtemplate. Ones a subtemplate is found, check if it is simple(choice).
If choice, continue though choice. One decoder checked the last option and
no match found, then check again if it is optional choice and if it is, pass
tag data to parent and adjust states.
Attachment #193111 -
Flags: review?(nelson)
Comment 2•19 years ago
|
||
Adding Bob to ASN.1 encoder/decoder related bugs
Comment 3•19 years ago
|
||
Comment on attachment 193111 [details] [diff] [review]
allow inline & optinal choice
I'm giving this patch r- for two specific reasons.
And I'm also asking Julien to review it, to determine if it is
correct, apart from the two issues I cite here.
>+ /* If INLINE and OPTIONAL bits are set, check that the subtemplate
>+ * is simple */
>+ if ((encode_kind & SEC_ASN1_INLINE) && optional) {
>+ if (PR_FALSE == SEC_ASN1IsTemplateSimple(subt)) {
>+ PORT_Assert(0); /* complex templates not handled as inline
>+ optional */
An assertion like this only works in a debug build. It does NOTHING in an
optimized build. So, each PORT_Assert call MUST be followed by some code
that is executed in non-debug builds that deals with the situation, in
whatever manner is appropriate (typically setting an error code and
returning or going to some error label). In those rare cases where it
is ok to just fall through and not take any action in the optimized
build for a case where the assertion would have failed, there needs to
be a comment explaining WHY it is OK to take no action there.
>+ }
>+ }
>+
> dest = (char *)child->dest - child->theTemplate->offset;
> child->theTemplate++;
>
> if (0 == child->theTemplate->kind) {
>- /* Ran out of choices */
>- PORT_SetError(SEC_ERROR_BAD_DER);
>- state->top->status = decodeError;
>- return (sec_asn1d_state *)NULL;
why is the indentation different in the patch than in the
previous code? It looks to be correct in the code being
removed. Maybe this is just another space/tab thing?
>+ /* Check if the choice is optional */
>+ if (state->optional != PR_TRUE) {
Always test for equality or inequality with PR_FALSE, not PR_TRUE,
e.g. test for == PR_FALSE, not for != PR_TRUE.
>+ /* Ran out of choices */
>+ PORT_SetError(SEC_ERROR_BAD_DER);
>+ state->top->status = decodeError;
>+ return (sec_asn1d_state *)NULL;
>+ } else {
This "else" is unnecessary because of the preceeding return.
Please remove the "else {, and remove the matching } and
unindent the lines in between.
>+ /* If it was optional, pass found tags to parent
>+ * clear child state and set state to afterChoice */
>+ state->missing = PR_TRUE;
>+ state->found_tag_modifiers = child->found_tag_modifiers;
>+ state->found_tag_number = child->found_tag_number;
>+ child->place = notInUse;
>+ state->place = afterChoice;
>+ return state;
Julien, please review the new blocks of code above for correctness.
Thanks.
>+ }
> }
Attachment #193111 -
Flags: review?(nelson)
Attachment #193111 -
Flags: review?(julien.pierre.bugs)
Attachment #193111 -
Flags: review-
Comment 4•19 years ago
|
||
The indentation problem is caused by tab vs. space
in diffs. Julien doesn't use tabs. Most NSS code,
including this file, uses tabs.
Comment 5•19 years ago
|
||
Wan-Teh,
Re: tabs, are we talking about the same files here ? I don't recall making
changes to the classic decoder, only to the encoder. According to cvs log, I did
touch the classic decoder 4 times in 2001/2002 ;) But I didn't write the latest
patch.
It is true that I tend to write new code with all spaces, especially new
functions or new files. For patches I try to use whatever was in the block of
code that I'm editing. However, as long as the sources display properly in all
editors with the tab size set to 8, I don't think we should be worried about
whether patches contain tabs or the appropriate number of spaces. diff tools
should be able to take care of the difference for us when reviewing patches.
We are spending an awful lot of time discussing and holding patches for
something that really should be a non-issue since the code is visually and
syntactically correct both ways.
Comment 6•19 years ago
|
||
Julien: I was just answering Nelson's question.
The tab vs. space issue doesn't bother me.
Comment 7•19 years ago
|
||
Sorry, Julien, I thought this is your patch.
I just realized that it's Alexei's.
Comment 8•19 years ago
|
||
Nelson,
Re: the assertion without error check, it appears that Alexei modeled his patch
after the one I made to the encoder in bug 280121, to which you gave r+ .
Please read my response in comment #9 of that bug.
If you know what error should be set and how to report it, please by all means
suggest it.
Comment 9•19 years ago
|
||
I suggest that in this case, the PORT_Assert(0) be followed by these lines:
PORT_SetError(SEC_ERROR_BAD_TEMPLATE);
return NULL;
Comment 10•19 years ago
|
||
Comment on attachment 193111 [details] [diff] [review]
allow inline & optinal choice
I have no additional comments on this patch besides what Nelson already reported.
Attachment #193111 -
Flags: review?(julien.pierre.bugs)
Updated•19 years ago
|
QA Contact: jason.m.reid → libraries
Reporter | ||
Updated•18 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.12
Comment 11•16 years ago
|
||
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---
Comment 12•2 years ago
|
||
The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: alvolkov.bgs → nobody
Flags: needinfo?(bbeurdouche)
Updated•2 years ago
|
Severity: normal → S3
Comment 13•2 years ago
|
||
We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.
Flags: needinfo?(bbeurdouche)
You need to log in
before you can comment on or make changes to this bug.
Description
•