From 0c2773039b6b6c2195c88a0a59dd26eda67d9924 Mon Sep 17 00:00:00 2001 From: ejurgensen Date: Thu, 5 Oct 2017 22:13:01 +0200 Subject: [PATCH] [-] Fix alsa.c null pointer deref + some minor bugs and do some housekeeping Thanks to Denis Denisov and cppcheck for notifying about the below. The leaks are edge cases, but the warning of dereference of avail in alsa.c points at a bug that could probably cause actual crashes. [src/evrtsp/rtsp.c:1352]: (warning) Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [src/httpd_daap.c:228]: (error) Memory leak: s [src/library.c:280]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'. [src/library.c:284]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'. [src/library/filescanner_playlist.c:251]: (error) Resource leak: fp [src/library/filescanner_playlist.c:273]: (error) Resource leak: fp [src/outputs/alsa.c:143]: (warning) Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [src/outputs/alsa.c:657]: (warning) Possible null pointer dereference: avail [src/outputs/dummy.c:75]: (warning) Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [src/outputs/fifo.c:245]: (warning) Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [src/outputs/raop.c:1806]: (warning) Assignment of function parameter has no effect outside the function. Did you forget dereferencing it? [src/outputs/raop.c:1371]: (warning) %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [src/outputs/raop.c:1471]: (warning) %u in format string (no. 1) requires 'unsigned int' but the argument type is 'signed int'. [src/outputs/raop_verification.c:705] -> [src/outputs/raop_verification.c:667]: (warning) Either the condition 'if(len_M)' is redundant or there is possible null pointer dereference: len_M. --- src/evrtsp/rtsp.c | 2 +- src/httpd_daap.c | 20 +++++++------------- src/library.c | 4 ++-- src/library/filescanner_playlist.c | 12 ++++++------ src/outputs/alsa.c | 8 +++++--- src/outputs/cast.c | 3 +++ src/outputs/dummy.c | 5 +++-- src/outputs/fifo.c | 4 +++- src/outputs/pulse.c | 3 +++ src/outputs/raop.c | 11 ++++++----- src/outputs/raop_verification.c | 6 ++---- 11 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/evrtsp/rtsp.c b/src/evrtsp/rtsp.c index d579b09f..ea8ccc5a 100644 --- a/src/evrtsp/rtsp.c +++ b/src/evrtsp/rtsp.c @@ -1349,7 +1349,7 @@ evrtsp_connection_get_local_address(struct evrtsp_connection *evcon, default: free(*address); - address = NULL; + *address = NULL; event_err(1, "%s: unhandled address family\n", __func__); return; diff --git a/src/httpd_daap.c b/src/httpd_daap.c index 0564275b..8a58f872 100644 --- a/src/httpd_daap.c +++ b/src/httpd_daap.c @@ -211,7 +211,7 @@ daap_session_add(const char *user_agent, int request_session_id) daap_session_cleanup(); - s = (struct daap_session *)malloc(sizeof(struct daap_session)); + s = calloc(1, sizeof(struct daap_session)); if (!s) { DPRINTF(E_LOG, L_DAAP, "Out of memory for DAAP session\n"); @@ -225,6 +225,7 @@ daap_session_add(const char *user_agent, int request_session_id) if (daap_session_get(request_session_id)) { DPRINTF(E_LOG, L_DAAP, "Session id requested in login (%d) is not available\n", request_session_id); + free(s); return NULL; } @@ -385,7 +386,7 @@ daap_sort_context_new(void) struct sort_ctx *ctx; int ret; - ctx = (struct sort_ctx *)malloc(sizeof(struct sort_ctx)); + ctx = calloc(1, sizeof(struct sort_ctx)); if (!ctx) { DPRINTF(E_LOG, L_DAAP, "Out of memory for sorting context\n"); @@ -532,14 +533,10 @@ user_agent_filter(const char *user_agent, struct query_params *qp) { const char *filter; char *buffer; - int len; if (!user_agent) return; - // Valgrind doesn't like strlen(filter) below, so instead we allocate 128 bytes - // to hold the string and the leading " AND ". Remember to adjust the 128 if - // you define strings here that will be too large for the buffer. if (is_remote(user_agent)) filter = "(f.data_kind <> 1)"; // No internet radio else @@ -547,12 +544,9 @@ user_agent_filter(const char *user_agent, struct query_params *qp) if (qp->filter) { - len = strlen(qp->filter) + 128; - buffer = (char *)malloc(len); - snprintf(buffer, len, "%s AND %s", qp->filter, filter); + buffer = safe_asprintf("%s AND %s", qp->filter, filter); free(qp->filter); - qp->filter = strdup(buffer); - free(buffer); + qp->filter = buffer; } else qp->filter = strdup(filter); @@ -719,7 +713,7 @@ parse_meta(struct evhttp_request *req, char *tag, const char *param, const struc DPRINTF(E_DBG, L_DAAP, "Asking for %d meta tags\n", nmeta); - meta = (const struct dmap_field **)malloc(nmeta * sizeof(const struct dmap_field *)); + meta = (const struct dmap_field **)calloc(nmeta, sizeof(const struct dmap_field *)); if (!meta) { DPRINTF(E_LOG, L_DAAP, "Could not allocate meta array; out of memory\n"); @@ -1069,7 +1063,7 @@ daap_reply_update(struct evhttp_request *req, struct evbuffer *evbuf, char **uri } /* Else, just let the request hang until we have changes to push back */ - ur = (struct daap_update_request *)malloc(sizeof(struct daap_update_request)); + ur = calloc(1, sizeof(struct daap_update_request)); if (!ur) { DPRINTF(E_LOG, L_DAAP, "Out of memory for update request\n"); diff --git a/src/library.c b/src/library.c index 8c57f85b..287cbe5e 100644 --- a/src/library.c +++ b/src/library.c @@ -277,11 +277,11 @@ fixup_tags(struct media_file_info *mfi) if (!mfi->album) { - len = snprintf(NULL, 0, "%s, Season %d", mfi->tv_series_name, mfi->tv_season_num); + len = snprintf(NULL, 0, "%s, Season %u", mfi->tv_series_name, mfi->tv_season_num); mfi->album = (char *)malloc(len + 1); if (mfi->album) - sprintf(mfi->album, "%s, Season %d", mfi->tv_series_name, mfi->tv_season_num); + sprintf(mfi->album, "%s, Season %u", mfi->tv_series_name, mfi->tv_season_num); } } diff --git a/src/library/filescanner_playlist.c b/src/library/filescanner_playlist.c index 76f10ff8..7704d13f 100644 --- a/src/library/filescanner_playlist.c +++ b/src/library/filescanner_playlist.c @@ -248,7 +248,7 @@ scan_playlist(char *file, time_t mtime, int dir_id) { DPRINTF(E_LOG, L_SCAN, "Out of memory\n"); - return; + goto out_close; } memset(pli, 0, sizeof(struct playlist_info)); @@ -270,7 +270,7 @@ scan_playlist(char *file, time_t mtime, int dir_id) DPRINTF(E_LOG, L_SCAN, "Error adding playlist '%s'\n", file); free_pli(pli, 0); - return; + goto out_close; } DPRINTF(E_INFO, L_SCAN, "Added playlist as id %d\n", pl_id); @@ -346,11 +346,11 @@ scan_playlist(char *file, time_t mtime, int dir_id) { DPRINTF(E_LOG, L_SCAN, "Error reading playlist '%s': %s\n", file, strerror(errno)); - fclose(fp); - return; + goto out_close; } - fclose(fp); - DPRINTF(E_INFO, L_SCAN, "Done processing playlist\n"); + + out_close: + fclose(fp); } diff --git a/src/outputs/alsa.c b/src/outputs/alsa.c index 8727c990..591f309d 100644 --- a/src/outputs/alsa.c +++ b/src/outputs/alsa.c @@ -133,14 +133,15 @@ prebuf_free(struct alsa_session *as) static void alsa_session_free(struct alsa_session *as) { + if (!as) + return; + event_free(as->deferredev); prebuf_free(as); free(as->output_session); free(as); - - as = NULL; } static void @@ -654,7 +655,8 @@ buffer_write(struct alsa_session *as, uint8_t *buf, snd_pcm_sframes_t *avail, in if (ret != nsamp) DPRINTF(E_WARN, L_LAUDIO, "ALSA partial write detected\n"); - *avail -= ret; + if (avail) + *avail -= ret; return 0; } diff --git a/src/outputs/cast.c b/src/outputs/cast.c index bc41ec0b..de9ff39e 100644 --- a/src/outputs/cast.c +++ b/src/outputs/cast.c @@ -509,6 +509,9 @@ squote_to_dquote(char *buf) static void cast_session_free(struct cast_session *cs) { + if (!cs) + return; + event_free(cs->reply_timeout); event_free(cs->ev); diff --git a/src/outputs/dummy.c b/src/outputs/dummy.c index e5dfad2e..3083ead4 100644 --- a/src/outputs/dummy.c +++ b/src/outputs/dummy.c @@ -67,12 +67,13 @@ defer_cb(int fd, short what, void *arg); static void dummy_session_free(struct dummy_session *ds) { + if (!ds) + return; + event_free(ds->deferredev); free(ds->output_session); free(ds); - - ds = NULL; } static void diff --git a/src/outputs/fifo.c b/src/outputs/fifo.c index dd44ceb8..0aa7c10e 100644 --- a/src/outputs/fifo.c +++ b/src/outputs/fifo.c @@ -237,12 +237,14 @@ fifo_empty(struct fifo_session *fifo_session) static void fifo_session_free(struct fifo_session *fifo_session) { + if (!fifo_session) + return; + event_free(fifo_session->deferredev); free(fifo_session->output_session); free(fifo_session); free_buffer(); - fifo_session = NULL; } static void diff --git a/src/outputs/pulse.c b/src/outputs/pulse.c index 859c0553..b41a27ab 100644 --- a/src/outputs/pulse.c +++ b/src/outputs/pulse.c @@ -97,6 +97,9 @@ pulse_from_device_volume(int device_volume) static void pulse_session_free(struct pulse_session *ps) { + if (!ps) + return; + if (ps->stream) { pa_threaded_mainloop_lock(pulse.mainloop); diff --git a/src/outputs/raop.c b/src/outputs/raop.c index beccb59c..fcc4b469 100644 --- a/src/outputs/raop.c +++ b/src/outputs/raop.c @@ -1368,7 +1368,7 @@ raop_send_req_flush(struct raop_session *rs, uint64_t rtptime, evrtsp_req_cb cb) } /* Restart sequence: last sequence + 1 */ - ret = snprintf(buf, sizeof(buf), "seq=%u;rtptime=%u", stream_seq + 1, RAOP_RTPTIME(rtptime)); + ret = snprintf(buf, sizeof(buf), "seq=%" PRIu16 ";rtptime=%u", stream_seq + 1, RAOP_RTPTIME(rtptime)); if ((ret < 0) || (ret >= sizeof(buf))) { DPRINTF(E_LOG, L_RAOP, "RTP-Info too big for buffer in FLUSH request\n"); @@ -1468,7 +1468,7 @@ raop_send_req_record(struct raop_session *rs, evrtsp_req_cb cb) evrtsp_add_header(req->output_headers, "Range", "npt=0-"); /* Start sequence: next sequence */ - ret = snprintf(buf, sizeof(buf), "seq=%u;rtptime=%u", stream_seq + 1, RAOP_RTPTIME(rs->start_rtptime)); + ret = snprintf(buf, sizeof(buf), "seq=%" PRIu16 ";rtptime=%u", stream_seq + 1, RAOP_RTPTIME(rs->start_rtptime)); if ((ret < 0) || (ret >= sizeof(buf))) { DPRINTF(E_LOG, L_RAOP, "RTP-Info too big for buffer in RECORD request\n"); @@ -1778,6 +1778,9 @@ raop_status(struct raop_session *rs) static void raop_session_free(struct raop_session *rs) { + if (!rs) + return; + evrtsp_connection_set_closecb(rs->ctrl, NULL, NULL); evrtsp_connection_free(rs->ctrl); @@ -1802,8 +1805,6 @@ raop_session_free(struct raop_session *rs) free(rs->output_session); free(rs); - - rs = NULL; } static void @@ -3461,7 +3462,7 @@ raop_v2_resend_range(struct raop_session *rs, uint16_t seqnum, uint16_t len) /* Check that seqnum is in the retransmit buffer */ if ((seqnum > pktbuf_head->seqnum) || (seqnum < pktbuf_tail->seqnum)) { - DPRINTF(E_WARN, L_RAOP, "Device '%s' asking for seqnum %u; not in buffer (h %u t %u)\n", rs->devname, seqnum, pktbuf_head->seqnum, pktbuf_tail->seqnum); + DPRINTF(E_WARN, L_RAOP, "Device '%s' asking for seqnum %" PRIu16 "; not in buffer (h %" PRIu16 " t %" PRIu16 ")\n", rs->devname, seqnum, pktbuf_head->seqnum, pktbuf_tail->seqnum); return; } diff --git a/src/outputs/raop_verification.c b/src/outputs/raop_verification.c index e697211d..e534d923 100644 --- a/src/outputs/raop_verification.c +++ b/src/outputs/raop_verification.c @@ -702,14 +702,12 @@ srp_user_process_challenge(struct SRPUser *usr, const unsigned char *bytes_s, in calculate_H_AMK(usr->alg, usr->H_AMK, usr->A, usr->M, usr->session_key, usr->session_key_len); *bytes_M = usr->M; - if (len_M) - *len_M = hash_length(usr->alg); + *len_M = hash_length(usr->alg); } else { *bytes_M = NULL; - if (len_M) - *len_M = 0; + *len_M = 0; } cleanup2: