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

Fix coverity warning #4003

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Fix coverity warning #4003

merged 3 commits into from
Jul 3, 2024

Conversation

trengginas
Copy link
Member

Warnings from coverity:

*** CID 1547500:  Integer handling issues  (INTEGER_OVERFLOW)
/pjsip/src/pjsip-ua/sip_100rel.c: 293 in pjsip_100rel_create_prack()
287             dd->uac_state_list = uac_state;
288         }
289     
290         /* If this is from new INVITE transaction, reset UAC state. */
291         if (rdata->msg_info.cseq->cseq != uac_state->cseq) {
292             uac_state->cseq = rdata->msg_info.cseq->cseq;
>>>     CID 1547500:  Integer handling issues  (INTEGER_OVERFLOW)
>>>     Expression "rseq - 1U", which is equal to 4294967295, where "rseq" is known to be equal to 0, underflows the type that receives it, an unsigned integer 32 bits wide.
293             uac_state->rseq = rseq - 1;
294         }
295     
296         /* Ignore provisional response retransmission */
297         if (rseq <= uac_state->rseq) {
298             /* This should have been handled before */

From ref, the RSeq value must be set to 1 to 2**32 -1.
The patch will check for the RSeq value and make sure it is greater than 1.

*** CID 1547498:  Error handling issues  (CHECKED_RETURN)
/pjmedia/src/pjmedia/endpoint.c: 806 in pjmedia_endpt_create_video_sdp()
800             /* Must support RTP packetization */
801             if ((codec_info[i].packings & PJMEDIA_VID_PACKING_PACKETS) == 0)
802             {
803                 continue;
804             }
805     
>>>     CID 1547498:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "pjmedia_vid_codec_mgr_get_default_param" without checking return value (as is done elsewhere 8 out of 10 times).
806             pjmedia_vid_codec_mgr_get_default_param(NULL, &codec_info[i],
807                                                     &codec_param);
808     
809             fmt = &m->desc.fmt[m->desc.fmt_count++];
810             fmt->ptr = (char*) pj_pool_alloc(pool, 8);
811             fmt->slen = pj_utoa(codec_info[i].pt, fmt->ptr);

Fix by adding check to the return value from pjmedia_vid_codec_mgr_get_default_param().

*** CID 1547497:  Incorrect expression  (COPY_PASTE_ERROR)
/pjlib/src/pjlib-test/ioq_tcp.c: 890 in compliance_test_2()
884                 server[i].sock = PJ_INVALID_SOCKET;
885             }
886     
887             if (client[i].key != NULL) {
888                 pj_ioqueue_unregister(client[i].key);
889                 client[i].key = NULL;
>>>     CID 1547497:  Incorrect expression  (COPY_PASTE_ERROR)
>>>     "server" in "server[i].sock" looks like a copy-paste error.
890                 server[i].sock = PJ_INVALID_SOCKET;
891             } else if (client[i].sock != PJ_INVALID_SOCKET) {
892                 pj_sock_close(client[i].sock);
893                 client[i].sock = PJ_INVALID_SOCKET;
894             }
895         }

It should be client sock.

Comment on lines 272 to 277
if (rseq < 1) {
PJ_LOG(4, (dd->inv->dlg->obj_name,
"Ignoring 100rel response RSeq header value less than 1"));
return PJSIP_EINVALIDMSG;
}

Copy link
Member

Choose a reason for hiding this comment

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

Actually as receiver, we better be lenient, especially if it is not so urgent/risky, perhaps printing warning is sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the message received with RSeq 0, currently it will be ignored in https://github.com/pjsip/pjproject/blob/coverity02/pjsip/src/pjsip-ua/sip_100rel.c#L303

Copy link
Member

Choose a reason for hiding this comment

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

so should we return PJ_EIGNORED here, so it's the same as the old behavior?

Copy link
Member

Choose a reason for hiding this comment

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

If the message received with RSeq 0, currently it will be ignored in https://github.com/pjsip/pjproject/blob/coverity02/pjsip/src/pjsip-ua/sip_100rel.c#L303

The pointed code does not seem to check for rseq==0, it checks for retransmission (where the original rseq may still be 0)? So IMO, perhaps better check the behavior when rseq ==0 (e.g: using SIPp) to at least avoid behavior change.

Copy link
Member Author

Choose a reason for hiding this comment

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

The rseq is unsigned, so the condition will always be true when rseq ==0 regardless the original rseq. If we want to avoid behavior change we should return PJ_EIGNORED (as @sauwming suggested) or just print the warning message.

@trengginas trengginas merged commit 0dac636 into master Jul 3, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants