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)

3.10

Tracking

(Not tracked)

People

(Reporter: alvolkov.bgs, Unassigned)

Details

Attachments

(1 file)

Original decoder, unlike quickDER, does not support this feature that makes tests generated with dynamic data generator fail on some templates. See also: 280121
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)
Adding Bob to ASN.1 encoder/decoder related bugs
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-
The indentation problem is caused by tab vs. space in diffs. Julien doesn't use tabs. Most NSS code, including this file, uses tabs.
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.
Julien: I was just answering Nelson's question. The tab vs. space issue doesn't bother me.
Sorry, Julien, I thought this is your patch. I just realized that it's Alexei's.
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.
I suggest that in this case, the PORT_Assert(0) be followed by these lines: PORT_SetError(SEC_ERROR_BAD_TEMPLATE); return NULL;
Blocks: 313605
No longer blocks: 313605
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)
QA Contact: jason.m.reid → libraries
Priority: -- → P2
Target Milestone: --- → 3.12
Unsetting target milestone in unresolved bugs whose targets have passed.
Target Milestone: 3.12 → ---

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)
Severity: normal → S3

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.

Attachment

General

Creator:
Created:
Updated:
Size: