Skip to content

http/fetch: Pass a non-null buffer to ne_set_request_body_buffer API#18

Open
kraj wants to merge 1 commit into
metabrainz:masterfrom
kraj:Wnonnull
Open

http/fetch: Pass a non-null buffer to ne_set_request_body_buffer API#18
kraj wants to merge 1 commit into
metabrainz:masterfrom
kraj:Wnonnull

Conversation

@kraj
Copy link
Copy Markdown

@kraj kraj commented Aug 25, 2022

Newer versions of neon has added a check for non-null arguments for
ne_set_request_body_buffer() API and this is triggered but older
compiler only treats -Wnonnull as warning so all was fine, however gcc
12.2 has started to throw this warning as error by default and builds
are breaking

Fixes
src/HTTPFetch.cc:186:38: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
ne_set_request_body_buffer(req,0,0);
~ ^

Signed-off-by: Khem Raj raj.khem@gmail.com

Newer versions of neon has added a check for non-null arguments for
ne_set_request_body_buffer() API and this is triggered but older
compiler only treats -Wnonnull as warning so all was fine, however gcc
12.2 has started to throw this warning as error by default and builds
are breaking

Fixes
src/HTTPFetch.cc:186:38: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
                        ne_set_request_body_buffer(req,0,0);
                                                       ~  ^

Signed-off-by: Khem Raj <raj.khem@gmail.com>
@michaelmoreton
Copy link
Copy Markdown

I'm a little confused by this change. The code is constructing an HTTP PUT message, and the existing code tries to append a zero length request body. It would appear that this is actually un-necessary, as the declaration has been changed to stop you passing a NULL buffer pointer.

The proposed change seems to be adding an arbitrary length, uninitialised buffer as the request body. While that will get rid of the compilation error, it seems like a dangerous thing to do.

Couldn't we just remove the call to ne_set_request_body_buffer(), or am I missing something? I've tried this in my local copy and it seems to work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants