diff mbox series

channel_curl: Initialize reply buffer

Message ID 20230413064309.4570-1-christian.storm@siemens.com
State Accepted
Headers show
Series channel_curl: Initialize reply buffer | expand

Commit Message

Storm, Christian April 13, 2023, 6:43 a.m. UTC 
Initialize reply buffer to empty string so that reply
logging doesn't print garbage if no body was received.

Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 corelib/channel_curl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Stefano Babic April 13, 2023, 12:45 p.m. UTC | #1 
On 13.04.23 08:43, 'Christian Storm' via swupdate wrote:
> Initialize reply buffer to empty string so that reply
> logging doesn't print garbage if no body was received.
> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   corelib/channel_curl.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
> index 79161a7d..528ae8ef 100644
> --- a/corelib/channel_curl.c
> +++ b/corelib/channel_curl.c
> @@ -926,6 +926,7 @@ static channel_op_res_t setup_reply_buffer(CURL *handle, write_callback_t *wrdat
>   		ERROR("Channel buffer reservation failed with OOM.");
>   		return CHANNEL_ENOMEM;
>   	}
> +	wrdata->outdata->memory = 0;
>   
>   	if ((curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION,
>   			      channel_callback_membuffer) != CURLE_OK) ||


Applied to -master, thanks !

Best regards,
Stefano Babic
Stefano Babic May 1, 2023, 1:05 p.m. UTC | #2 
Hi Christian,

On 13.04.23 14:45, Stefano Babic wrote:
> On 13.04.23 08:43, 'Christian Storm' via swupdate wrote:
>> Initialize reply buffer to empty string so that reply
>> logging doesn't print garbage if no body was received.
>>
>> Signed-off-by: Christian Storm <christian.storm@siemens.com>
>> ---
>>   corelib/channel_curl.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
>> index 79161a7d..528ae8ef 100644
>> --- a/corelib/channel_curl.c
>> +++ b/corelib/channel_curl.c
>> @@ -926,6 +926,7 @@ static channel_op_res_t setup_reply_buffer(CURL 
>> *handle, write_callback_t *wrdat
>>           ERROR("Channel buffer reservation failed with OOM.");
>>           return CHANNEL_ENOMEM;
>>       }
>> +    wrdata->outdata->memory = 0;
>>       if ((curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION,
>>                     channel_callback_membuffer) != CURLE_OK) ||
> 
> 
> Applied to -master, thanks !
> 

I had to revert it due to side effects. I tested current TOT together 
with Hawkbit, and updates fail. SWUpdate stops running and reports:


May 01 12:51:30 swupdate.sh[544]: [TRACE] : SWUPDATE running : 
[channel_log_reply] : Channel operation returned HTTP status code 200.

May 01 12:51:30 beaglebone-yocto swupdate.sh[544]: [ERROR] : SWUPDATE 
failed [0] ERROR corelib/channel_curl.c : parse_reply : 944 : Channel 
reply buffer was not filled.

May 01 12:51:30 swupdate.sh[544]: [ERROR] : SWUPDATE failed [0] ERROR 
suricatta/server_hawkbit.c : server_install_update : 1399 : Error while 
reporting installation progress to server.

After running bisect, I found that the small patch here is the cause. I 
revert it for now (I have currently no time to search for the cause), 
this should be better investigated.

Best regards,
Stefano
Storm, Christian May 2, 2023, 9:03 a.m. UTC | #3 
> Hi Stefano,

> > > Initialize reply buffer to empty string so that reply
> > > logging doesn't print garbage if no body was received.
> > > 
> > > Signed-off-by: Christian Storm <christian.storm@siemens.com>
> > > ---
> > >   corelib/channel_curl.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
> > > index 79161a7d..528ae8ef 100644
> > > --- a/corelib/channel_curl.c
> > > +++ b/corelib/channel_curl.c
> > > @@ -926,6 +926,7 @@ static channel_op_res_t setup_reply_buffer(CURL
> > > *handle, write_callback_t *wrdat
> > >           ERROR("Channel buffer reservation failed with OOM.");
> > >           return CHANNEL_ENOMEM;
> > >       }
> > > +    wrdata->outdata->memory = 0;
> > >       if ((curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION,
> > >                     channel_callback_membuffer) != CURLE_OK) ||
> > 
> > 
> > Applied to -master, thanks !
> > 
> 
> I had to revert it due to side effects. I tested current TOT together with
> Hawkbit, and updates fail. SWUpdate stops running and reports:
>
>
> May 01 12:51:30 swupdate.sh[544]: [TRACE] : SWUPDATE running :
> [channel_log_reply] : Channel operation returned HTTP status code 200.
> 
> May 01 12:51:30 beaglebone-yocto swupdate.sh[544]: [ERROR] : SWUPDATE failed
> [0] ERROR corelib/channel_curl.c : parse_reply : 944 : Channel reply buffer
> was not filled.
> 
> May 01 12:51:30 swupdate.sh[544]: [ERROR] : SWUPDATE failed [0] ERROR
> suricatta/server_hawkbit.c : server_install_update : 1399 : Error while
> reporting installation progress to server.
> 
> After running bisect, I found that the small patch here is the cause. I revert
> it for now (I have currently no time to search for the cause), this should be
> better investigated.

This patch is indeed nonsense and has side-effects, thanks for finding it!
Actually, it does make wrdata->outdata->memory a NULL pointer which is
"fine" as it doesn't print log garbage in this case and works in my
other test cases. This is, however, just because of the realloc() in 
corelib/channel_curl.c:channel_callback_membuffer() which makes ->memory
a non-NULL pointer again.... It should have read
  *wrdata->outdata->memory = '\0';

I'll send a v2. Thanks again!


Kind regards,
   Christian
Stefano Babic May 2, 2023, 9:07 a.m. UTC | #4 
On 02.05.23 11:03, 'Christian Storm' via swupdate wrote:
>> Hi Stefano,
> 
>>>> Initialize reply buffer to empty string so that reply
>>>> logging doesn't print garbage if no body was received.
>>>>
>>>> Signed-off-by: Christian Storm <christian.storm@siemens.com>
>>>> ---
>>>>    corelib/channel_curl.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
>>>> index 79161a7d..528ae8ef 100644
>>>> --- a/corelib/channel_curl.c
>>>> +++ b/corelib/channel_curl.c
>>>> @@ -926,6 +926,7 @@ static channel_op_res_t setup_reply_buffer(CURL
>>>> *handle, write_callback_t *wrdat
>>>>            ERROR("Channel buffer reservation failed with OOM.");
>>>>            return CHANNEL_ENOMEM;
>>>>        }
>>>> +    wrdata->outdata->memory = 0;
>>>>        if ((curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION,
>>>>                      channel_callback_membuffer) != CURLE_OK) ||
>>>
>>>
>>> Applied to -master, thanks !
>>>
>>
>> I had to revert it due to side effects. I tested current TOT together with
>> Hawkbit, and updates fail. SWUpdate stops running and reports:
>>
>>
>> May 01 12:51:30 swupdate.sh[544]: [TRACE] : SWUPDATE running :
>> [channel_log_reply] : Channel operation returned HTTP status code 200.
>>
>> May 01 12:51:30 beaglebone-yocto swupdate.sh[544]: [ERROR] : SWUPDATE failed
>> [0] ERROR corelib/channel_curl.c : parse_reply : 944 : Channel reply buffer
>> was not filled.
>>
>> May 01 12:51:30 swupdate.sh[544]: [ERROR] : SWUPDATE failed [0] ERROR
>> suricatta/server_hawkbit.c : server_install_update : 1399 : Error while
>> reporting installation progress to server.
>>
>> After running bisect, I found that the small patch here is the cause. I revert
>> it for now (I have currently no time to search for the cause), this should be
>> better investigated.
> 
> This patch is indeed nonsense and has side-effects, thanks for finding it!

...I found the side effects....

> Actually, it does make wrdata->outdata->memory a NULL pointer which is
> "fine" as it doesn't print log garbage in this case and works in my
> other test cases. This is, however, just because of the realloc() in
> corelib/channel_curl.c:channel_callback_membuffer() which makes ->memory
> a non-NULL pointer again.... It should have read
>    *wrdata->outdata->memory = '\0';
> 
> I'll send a v2. Thanks again!

Fine.

Regards,
Stefano
diff mbox series

Patch

diff --git a/corelib/channel_curl.c b/corelib/channel_curl.c
index 79161a7d..528ae8ef 100644
--- a/corelib/channel_curl.c
+++ b/corelib/channel_curl.c
@@ -926,6 +926,7 @@  static channel_op_res_t setup_reply_buffer(CURL *handle, write_callback_t *wrdat
 		ERROR("Channel buffer reservation failed with OOM.");
 		return CHANNEL_ENOMEM;
 	}
+	wrdata->outdata->memory = 0;
 
 	if ((curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION,
 			      channel_callback_membuffer) != CURLE_OK) ||