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

lib/gis: Add portable G_strlcpy function #4101

Merged
merged 5 commits into from
Jul 29, 2024
Merged

Conversation

lbartoletti
Copy link
Contributor

From commit message:

This commit introduces a new G_strlcpy function in lib/gis, inspired by
G_asprintf. G_strlcpy provides a safer alternative to strcpy and strncpy,
with consistent behavior across different systems.

Key points:

The function is implemented to use the native strlcpy where available,
falling back to our portable version on systems without it. This ensures
optimal performance on BSD systems while maintaining compatibility across
different platforms.

By providing G_strlcpy, we aim to improve the overall safety and
consistency of string operations in our codebase.

Marked as draft for now, since only tested on FreeBSD, which have strlcpy of course 😉

Supersed #4087 for the "test"

@github-actions github-actions bot added raster Related to raster data processing C Related code is in C libraries module labels Jul 25, 2024
@lbartoletti
Copy link
Contributor Author

@ShubhamDesai @echoix everywhere a strcpy is replaced by a strncpy + null termination, you could use the BSD (non-portable) strlcpy function.

https://man.freebsd.org/cgi/man.cgi?strlcpy(3)

@lbartoletti
Copy link
Contributor Author

checking for strlcpy... no for Ubuntu and yes for macOS (BSD-like) as expected. And r.path builds, nice!

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great addition, after just reading the code I have some suggestions.

lib/gis/strlcpy.c Outdated Show resolved Hide resolved
lib/gis/strlcpy.c Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
include/grass/defs/gis.h Outdated Show resolved Hide resolved
@echoix
Copy link
Member

echoix commented Jul 25, 2024

How does windows support look like? (Passing CI doesn't mean anything for now)

@lbartoletti lbartoletti marked this pull request as ready for review July 25, 2024 12:29
@nilason
Copy link
Contributor

nilason commented Jul 25, 2024

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

@lbartoletti
Copy link
Contributor Author

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

restrict is on FreeBSD version, but not used on other version, for example, the original OpenBSD https://github.com/libressl/openbsd/blob/master/src/lib/libc/string/strlcpy.c#L28

@lbartoletti
Copy link
Contributor Author

How does windows support look like? (Passing CI doesn't mean anything for now)

Like for Linux, Windows doesn't have strlcpy and will use "our" function. And, it's ok, also on Windows, since it's use only C standard function.

@nilason
Copy link
Contributor

nilason commented Jul 25, 2024

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

restrict is on FreeBSD version, but not used on other version, for example, the original OpenBSD https://github.com/libressl/openbsd/blob/master/src/lib/libc/string/strlcpy.c#L28

Yes, it is not required, but a compiler optimisation which we now can use (since we are supporting C11 standard). I don't think it is necessary to declare it in the prototype (gis.h).

@lbartoletti lbartoletti force-pushed the add_strlcpy branch 2 times, most recently from a2088a1 to 635f8a0 Compare July 25, 2024 12:52
@lbartoletti
Copy link
Contributor Author

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

restrict is on FreeBSD version, but not used on other version, for example, the original OpenBSD https://github.com/libressl/openbsd/blob/master/src/lib/libc/string/strlcpy.c#L28

Yes, it is not required, but a compiler optimisation which we now can use (since we are supporting C11 standard). I don't think it is necessary to declare it in the prototype (gis.h).

In fact, I think, __restrict is better for C/C++ https://learn.microsoft.com/fr-fr/cpp/cpp/extension-restrict?view=msvc-170 (and other like wikipedia etc)

@nilason
Copy link
Contributor

nilason commented Jul 25, 2024

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

restrict is on FreeBSD version, but not used on other version, for example, the original OpenBSD https://github.com/libressl/openbsd/blob/master/src/lib/libc/string/strlcpy.c#L28

Yes, it is not required, but a compiler optimisation which we now can use (since we are supporting C11 standard). I don't think it is necessary to declare it in the prototype (gis.h).

In fact, I think, __restrict is better for C/C++ https://learn.microsoft.com/fr-fr/cpp/cpp/extension-restrict?view=msvc-170 (and other like wikipedia etc)

__restrict/__restrict__ etc are compiler specific, and may or may not work. We'd better keep us to the standard.

@nilason
Copy link
Contributor

nilason commented Jul 25, 2024

restrict doesn't work well with C++, we can try remove it completely from the gis.h file only, but keep it in strlcpy.c. If that doesn't work it may be removed altogether.

restrict is on FreeBSD version, but not used on other version, for example, the original OpenBSD https://github.com/libressl/openbsd/blob/master/src/lib/libc/string/strlcpy.c#L28

Yes, it is not required, but a compiler optimisation which we now can use (since we are supporting C11 standard). I don't think it is necessary to declare it in the prototype (gis.h).

In fact, I think, __restrict is better for C/C++ https://learn.microsoft.com/fr-fr/cpp/cpp/extension-restrict?view=msvc-170 (and other like wikipedia etc)

__restrict/__restrict__ etc are compiler specific, and may or may not work. We'd better keep us to the standard.

An alternative way, could possibly be to use:

extern "C"
{
  #include "grass/gis.h"
}

in all C++ files. Like:

extern "C" {
#include <grass/gis.h>
#include <grass/raster.h>
#include <grass/glocale.h>
#include <grass/rbtree.h>
}

Or add a:

#ifdef __cplusplus
extern "C"
{
#endif

// original content of gis.h

#ifdef __cplusplus
}
#endif

then we can probably use restrict in gis.h as well (which would be preferable for clarity).

The former (the extern "C" version for cpp files), if this solves the problem with the restrict keyword, is probably the better solution anyway.

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the following suggested changes also C++ file can include gis.h. 'export "C"' doesn't to the trick.

(Historically, prototypes in GRASS doesn't have named parameters.)

lib/gis/strlcpy.c Outdated Show resolved Hide resolved
include/grass/defs/gis.h Outdated Show resolved Hide resolved
@lbartoletti
Copy link
Contributor Author

With the following suggested changes also C++ file can include gis.h. 'export "C"' doesn't to the trick.

(Historically, prototypes in GRASS doesn't have named parameters.)

Nice, thanks!

Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ready to merge this if there are no objections.

I have a very minor stylistic suggestion below. And since we have a squash-and-merge policy, I would prefer if you'd remove the r.path changes, to keep clean commits (it may be addressed with #4087).

lib/gis/strlcpy.c Outdated Show resolved Hide resolved
lbartoletti and others added 5 commits July 29, 2024 14:40
This commit introduces a new G_strlcpy function in lib/gis, inspired by
G_asprintf. G_strlcpy provides a safer alternative to strcpy and strncpy,
with consistent behavior across different systems.

Key points:
- Implements strlcpy functionality, available natively on BSD systems
- Portable implementation for non-BSD systems (excluding Linux with libbsd)
- Based on FreeBSD's implementation:
  https://github.com/freebsd/freebsd-src/blob/98dd639c94f716858ae29958f484729b1d2fd387/sys/libkern/strlcpy.c#L28
- Designed to replace unsafe uses of strcpy and strncpy throughout the project

The function is implemented to use the native strlcpy where available,
falling back to our portable version on systems without it. This ensures
optimal performance on BSD systems while maintaining compatibility across
different platforms.

By providing G_strlcpy, we aim to improve the overall safety and
consistency of string operations in our codebase.
…files to include gis.h without 'extern "C"' trick. Update function prototypes to use named parameters.

Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
@lbartoletti
Copy link
Contributor Author

I'm ready to merge this if there are no objections.

I have a very minor stylistic suggestion below. And since we have a squash-and-merge policy, I would prefer if you'd remove the r.path changes, to keep clean commits (it may be addressed with #4087).

Thanks. I removed the r.path changes

@echoix echoix mentioned this pull request Jul 29, 2024
@nilason nilason enabled auto-merge (squash) July 29, 2024 16:59
@nilason nilason merged commit 84db88a into OSGeo:main Jul 29, 2024
26 checks passed
@nilason
Copy link
Contributor

nilason commented Jul 29, 2024

@lbartoletti Thanks!!

@nilason nilason added this to the 8.5.0 milestone Jul 29, 2024
@nilason nilason added the enhancement New feature or request label Jul 29, 2024
@lbartoletti lbartoletti deleted the add_strlcpy branch July 29, 2024 17:41
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
This commit introduces a new G_strlcpy function in lib/gis, inspired by
G_asprintf. G_strlcpy provides a safer alternative to strcpy and strncpy,
with consistent behavior across different systems.

Key points:
- Implements strlcpy functionality, available natively on BSD systems
- Portable implementation for non-BSD systems (excluding Linux with libbsd)
- Based on FreeBSD's implementation:
  https://github.com/freebsd/freebsd-src/blob/98dd639c94f716858ae29958f484729b1d2fd387/sys/libkern/strlcpy.c#L28
- Designed to replace unsafe uses of strcpy and strncpy throughout the project

The function is implemented to use the native strlcpy where available,
falling back to our portable version on systems without it. This ensures
optimal performance on BSD systems while maintaining compatibility across
different platforms.

By providing G_strlcpy, we aim to improve the overall safety and
consistency of string operations in our codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C enhancement New feature or request libraries module raster Related to raster data processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants