From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id B480A1B3A5 for ; Tue, 23 Oct 2018 10:43:03 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2018 01:43:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,415,1534834800"; d="scan'208";a="243582217" Received: from klaatz-mobl.ger.corp.intel.com (HELO [10.237.220.107]) ([10.237.220.107]) by orsmga004.jf.intel.com with ESMTP; 23 Oct 2018 01:43:00 -0700 To: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , dev@dpdk.org Cc: harry.van.haaren@intel.com, stephen@networkplumber.org, gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, thomas@monjalon.net, bruce.richardson@intel.com, Ciara Power , Brian Archbold References: <20181016155802.2067-1-kevin.laatz@intel.com> <20181022110014.82153-1-kevin.laatz@intel.com> <20181022110014.82153-6-kevin.laatz@intel.com> From: "Laatz, Kevin" Message-ID: Date: Tue, 23 Oct 2018 09:42:58 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v6 05/13] telemetry: add client feature and sockets X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Oct 2018 08:43:04 -0000 Thanks for the review Mattias. Will address the comments both here and in the 4/13 review. Best regards, Kevin On 22/10/2018 15:05, Mattias Rönnblom wrote: > On 2018-10-22 13:00, Kevin Laatz wrote: >> >>   @@ -131,6 +217,38 @@ rte_telemetry_initial_accept(struct >> telemetry_impl *telemetry) >>   } >>     static int32_t >> +rte_telemetry_read_client(struct telemetry_impl *telemetry) >> +{ >> +    char buf[BUF_SIZE]; >> +    int ret, buffer_read = 0; > > No need to set it to zero. > >> + >> +    buffer_read = read(telemetry->accept_fd, buf, BUF_SIZE-1); >> + >> +    if (buffer_read == -1) { >> +        TELEMETRY_LOG_ERR("Read error"); >> +        return -1; >> +    } else if (buffer_read == 0) { >> +        goto close_socket; >> +    } else { > > I would have moved the 'ret' variable to this scope. > >> >> +static int32_t >> +rte_telemetry_read_client_sockets(struct telemetry_impl *telemetry) >> +{ >> +    telemetry_client *client; >> +    char client_buf[BUF_SIZE]; >> +    int bytes; >> + >> +    TAILQ_FOREACH(client, &telemetry->client_list_head, client_list) { >> +        bytes = read(client->fd, client_buf, BUF_SIZE-1); >> +        client_buf[bytes] = '\0'; > > read() might return -1, and you'll be writing out-of-bounds. > >> >> +int32_t >> +rte_telemetry_parse_client_message(struct telemetry_impl *telemetry, >> char *buf) >> +{ >> +    int ret, action_int; >> +    json_error_t error; >> +    json_t *root = json_loads(buf, 0, &error); >> + >> +    if (root == NULL) { >> +        TELEMETRY_LOG_WARN("Could not load JSON object from data >> passed in : %s", >> +                error.text); >> +        goto fail; >> +    } else if (!json_is_object(root)) { >> +        TELEMETRY_LOG_WARN("JSON Request is not a JSON object"); >> +        json_decref(root); >> +        goto fail; >> +    } >> + >> +    json_t *action = json_object_get(root, "action"); >> +    if (action == NULL) { >> +        TELEMETRY_LOG_WARN("Request does not have action field"); >> +        goto fail; >> +    } else if (!json_is_integer(action)) { >> +        TELEMETRY_LOG_WARN("Action value is not an integer"); >> +        goto fail; >> +    } >> + >> +    json_t *command = json_object_get(root, "command"); >> +    if (command == NULL) { >> +        TELEMETRY_LOG_WARN("Request does not have command field"); >> +        goto fail; >> +    } else if (!json_is_string(command)) { >> +        TELEMETRY_LOG_WARN("Command value is not a string"); >> +        goto fail; >> +    } >> + >> +    action_int = json_integer_value(action); >> +    if (action_int != ACTION_POST) { >> +        TELEMETRY_LOG_WARN("Invalid action code"); >> +        goto fail; >> +    } >> + >> +    if (strcmp(json_string_value(command), "clients") != 0) { >> +        TELEMETRY_LOG_WARN("Invalid command"); >> +        goto fail; >> +    } >> + >> +    json_t *data = json_object_get(root, "data"); >> +    if (data == NULL) { >> +        TELEMETRY_LOG_WARN("Request does not have data field"); >> +        goto fail; >> +    } >> + >> +    json_t *client_path = json_object_get(data, "client_path"); >> +    if (client_path == NULL) { >> +        TELEMETRY_LOG_WARN("Request does not have client_path field"); >> +        goto fail; >> +    } >> + >> +    if (!json_is_string(client_path)) { >> +        TELEMETRY_LOG_WARN("Client_path value is not a string"); >> +        goto fail; >> +    } >> + >> +    ret = rte_telemetry_register_client(telemetry, >> +            json_string_value(client_path)); >> +    if (ret < 0) { >> +        TELEMETRY_LOG_ERR("Could not register client"); >> +        telemetry->register_fail_count++; >> +        goto fail; >> +    } >> + > > You're leaking the root object here, but maybe that's fixed in > subsequent patches. > >> +    return 0; >> + >> +fail: >> +    TELEMETRY_LOG_WARN("Client attempted to register with invalid >> message"); >> +    return -1; >> +} >> +