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

Fixed size buffers #9

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Fixed size buffers #9

merged 3 commits into from
Apr 7, 2021

Conversation

mazes-80
Copy link
Contributor

@mazes-80 mazes-80 commented Apr 7, 2021

First pass to eliminate fixed size buffers (4 done).

There is 3 others places in the code where it happens, but my fixes involves GString
As your comments say you removed all references to GString, I didn't wanted to push them in this first pass.

@wdlkmpx
Copy link
Owner

wdlkmpx commented Apr 7, 2021

I don't want GString, it's a regression. I used fixed buffers to optimize performance for some old 1995 pcs,
In some cases it's not an issue, e.g: reporting of errors

I'm aware of g_strconcat and other functions

@mazes-80
Copy link
Contributor Author

mazes-80 commented Apr 7, 2021

allocating may be a performance issue on some old hardware I agree, and fixed size buffer isn't that bad sometimes but the one in history.c really is an issue.

Can you be tell more about your aversion for GString, I don't really get why you don't want them.

@wdlkmpx
Copy link
Owner

wdlkmpx commented Apr 7, 2021

I think it's about applying stuff in a conservative way

For example. when searching the history, the app does not expect you to type a whole Word document, thus the fixed buffer of 1024 bytes (it could be 2048)

Using that buffer in an intelligent way is better than allocating buffers hundreds of times.

@mazes-80
Copy link
Contributor Author

mazes-80 commented Apr 7, 2021

The reason I began gtk3, is my old (0.9.2) version of gmrun producing "segfaults" any time I launch it.
I had a shell one-liner in the history way over the limit of the buffer.

@wdlkmpx
Copy link
Owner

wdlkmpx commented Apr 7, 2021

These changes are ok I think, but searching the history requires you to type characters one by one, so you're going to type more than 512 or 1024 characters, it's about being reasonable

A buffer overflow cannot happen, the code prevents it by checking boundaries.

@mazes-80
Copy link
Contributor Author

mazes-80 commented Apr 7, 2021

Ah, yes fgets, still it impose limit to the size of lines in the history file

@wdlkmpx wdlkmpx merged commit a30b236 into wdlkmpx:master Apr 7, 2021
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