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

curl: use dynbuf for config file buffer management #5946

Closed
wants to merge 5 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 9, 2020

By making the dynbuf functions available as curlx_ verisons, the config file parser in the tool code can use that for its buffer management and for saver reallocs etc.

Once landed, we can move more curl functions over to dynbuf,

(Alternative take to #5941 )

@jay
Copy link
Member

jay commented Sep 10, 2020

Sharing memory allocation functions between the tool and library is tricky. In Windows the tool and library can each be using a separate CRT if libcurl is a DLL and therefore have separate memory handling. Also it would probably be slower to call into the DLL for each dynbuf memory call.

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Yes, but this isn't meant to share allocation functions. It is meant to share the dynbuf.c code and the allocations done by the tool should be done like normal tool-side allocations. Still a bit tricky to get right in the build systems but I think I'm almost there now...

@jay
Copy link
Member

jay commented Sep 10, 2020

If the code is built separately for the tool and library and there's naming conflicts and the tool and library can't both use the same function name (I seem to remember that's happened before) then you could change the function name depending on BUILDING_LIBCURL like

#ifdef BUILDING_LIBCURL
#define DYN_FNAME(x) libcurl_ ## x
#else
#define DYN_FNAME(x) x
#endif

void DYN_FNAME(dyn_nappend) (...

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Ah right, it will be a problem in static builds...

@bagder bagder marked this pull request as draft September 10, 2020 07:39
@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

I opted to plain redefined function names for the tool. I think it looks and feels better.

@bagder
Copy link
Member Author

bagder commented Sep 10, 2020

Can anyone figure out why the visual studio project build fails?

tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_init referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_free referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_add referenced in function my_get_line
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_reset referenced in function parseconfig
tool_parsecfg.obj : error LNK2019: unresolved external symbol curlx_dyn_ptr referenced in function parseconfig
  1. It was winbuild, not visual studio
  2. I figured it out

@bagder bagder marked this pull request as ready for review September 10, 2020 21:36
@bagder
Copy link
Member Author

bagder commented Sep 11, 2020

This looks fine now and I'll land this (unless someone objects) within a day or two.

src/tool_parsecfg.c Outdated Show resolved Hide resolved
src/tool_parsecfg.c Outdated Show resolved Hide resolved
src/tool_parsecfg.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member Author

bagder commented Sep 11, 2020

Yet another force-push

src/tool_parsecfg.c Outdated Show resolved Hide resolved
... fixes an integer overflow at the same time.

Reported-by: ihsinme on github
Assisted-by: Jay Satiro
@bagder
Copy link
Member Author

bagder commented Sep 13, 2020

Okay, pushed a new version that makes curl actually return an error back out if it runs into an out of memory.

src/tool_parsecfg.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member Author

bagder commented Sep 13, 2020

If you are only having a single line in the buffer at any time then that is not necessary you could just reset it.

This logic is doing exactly one line at a time...

@bagder bagder closed this in c4ea71a Sep 14, 2020
bagder added a commit that referenced this pull request Sep 14, 2020
... fixes an integer overflow at the same time.

Reported-by: ihsinme on github
Assisted-by: Jay Satiro

Closes #5946
bagder added a commit that referenced this pull request Sep 14, 2020
bagder added a commit that referenced this pull request Sep 14, 2020
@bagder bagder deleted the bagder/curl-dynbuf branch September 25, 2020 13:37
@Jan-E
Copy link
Contributor

Jan-E commented Oct 14, 2020

The fix for this seems to break the standard commandline that the PHP devs use: winlibs/cURL#16

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

Successfully merging this pull request may close these issues.

None yet

3 participants