From f087e771cb27c19b21de65fd0c7d7876cd2d8d54 Mon Sep 17 00:00:00 2001 From: komark06 <87216234+komark06@users.noreply.github.com> Date: Sun, 17 Mar 2024 19:57:36 +0800 Subject: [PATCH 01/11] Hook calloc for test harness (#172) This commit introduces a generic alloc function to handle both test_malloc and test_calloc operations based on the provided alloc_func_t. Additionally, it implements the test_calloc function to allocate memory and initialize it to zero. --- harness.c | 39 +++++++++++++++++++++++++++++---------- harness.h | 1 + 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/harness.c b/harness.c index 475a89455..4e02891f5 100644 --- a/harness.c +++ b/harness.c @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -58,6 +59,12 @@ static jmp_buf env; static volatile sig_atomic_t jmp_ready = false; static bool time_limited = false; +/* For test_malloc and test_calloc */ +typedef enum { + TEST_MALLOC, + TEST_CALLOC, +} alloc_t; + /* Internal functions */ /* Should this allocation fail? */ @@ -115,17 +122,23 @@ static size_t *find_footer(block_element_t *b) return p; } -/* Implementation of application functions */ - -void *test_malloc(size_t size) +static void *alloc(alloc_t alloc_type, size_t size) { if (noallocate_mode) { - report_event(MSG_FATAL, "Calls to malloc disallowed"); + char *msg_alloc_forbidden[] = { + "Calls to malloc are disallowed", + "Calls to calloc are disallowed", + }; + report_event(MSG_FATAL, "%s", msg_alloc_forbidden[alloc_type]); return NULL; } if (fail_allocation()) { - report_event(MSG_WARN, "Malloc returning NULL"); + char *msg_alloc_failure[] = { + "Malloc returning NULL", + "Calloc returning NULL", + }; + report_event(MSG_WARN, "%s", msg_alloc_failure[alloc_type]); return NULL; } @@ -142,7 +155,7 @@ void *test_malloc(size_t size) new_block->payload_size = size; *find_footer(new_block) = MAGICFOOTER; void *p = (void *) &new_block->payload; - memset(p, FILLCHAR, size); + memset(p, !alloc_type * FILLCHAR, size); // cppcheck-suppress nullPointerRedundantCheck new_block->next = allocated; // cppcheck-suppress nullPointerRedundantCheck @@ -156,16 +169,22 @@ void *test_malloc(size_t size) return p; } +/* Implementation of application functions */ + +void *test_malloc(size_t size) +{ + return alloc(TEST_MALLOC, size); +} + // cppcheck-suppress unusedFunction void *test_calloc(size_t nelem, size_t elsize) { /* Reference: Malloc tutorial * https://danluu.com/malloc-tutorial/ */ - size_t size = nelem * elsize; // TODO: check for overflow - void *ptr = test_malloc(size); - memset(ptr, 0, size); - return ptr; + if (!nelem || !elsize || nelem > SIZE_MAX / elsize) + return NULL; + return alloc(TEST_CALLOC, nelem * elsize); } void test_free(void *p) diff --git a/harness.h b/harness.h index 8696b630e..3e2ccec40 100644 --- a/harness.h +++ b/harness.h @@ -55,6 +55,7 @@ void trigger_exception(char *msg); /* Tested program use our versions of malloc and free */ #define malloc test_malloc +#define calloc test_calloc #define free test_free /* Use undef to avoid strdup redefined error */ From c44d8e2256d7756ce6a6c96fe7ef86598bfb46d8 Mon Sep 17 00:00:00 2001 From: Lumynous Date: Mon, 18 Mar 2024 19:45:51 +0800 Subject: [PATCH 02/11] Clarify the operation about deleting duplicates (#174) The original comment states that false is returned when list head is NULL. However, this is inconsistent with functions such as 'q_delete_mid' and has been confirmed to be a typo. --- queue.h | 2 +- scripts/checksums | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/queue.h b/queue.h index 09dd3adfb..89dd99304 100644 --- a/queue.h +++ b/queue.h @@ -151,7 +151,7 @@ bool q_delete_mid(struct list_head *head); * Reference: * https://leetcode.com/problems/remove-duplicates-from-sorted-list-ii/ * - * Return: true for success, false if list is NULL. + * Return: true for success, false if list is NULL or empty. */ bool q_delete_dup(struct list_head *head); diff --git a/scripts/checksums b/scripts/checksums index 521c3774e..175038198 100644 --- a/scripts/checksums +++ b/scripts/checksums @@ -1,2 +1,2 @@ -444bab14cb8ad4c949f61b3c10a22a3ed2401425 queue.h +186d420b53e21c6daf8eabe980a5b90780c53bcd queue.h 3337dbccc33eceedda78e36cc118d5a374838ec7 list.h From 7a02fb25796fcfffbea7e8406af0ff14e9b4ce02 Mon Sep 17 00:00:00 2001 From: Lin Chun Yeh Date: Sun, 10 Mar 2024 22:08:17 +0800 Subject: [PATCH 03/11] Clarify ascend/descend description Add supllementary explainations for q_ascend and q_descend, indicating the required memory operations that must be implemented in these functions. --- queue.h | 2 ++ scripts/checksums | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/queue.h b/queue.h index 89dd99304..8d1823036 100644 --- a/queue.h +++ b/queue.h @@ -207,6 +207,7 @@ void q_sort(struct list_head *head, bool descend); * * No effect if queue is NULL or empty. If there has only one element, do * nothing. + * Memory allocated to removed nodes must be freed. * * Reference: * https://leetcode.com/problems/remove-nodes-from-linked-list/ @@ -222,6 +223,7 @@ int q_ascend(struct list_head *head); * * No effect if queue is NULL or empty. If there has only one element, do * nothing. + * Memory allocated to removed nodes must be freed. * * Reference: * https://leetcode.com/problems/remove-nodes-from-linked-list/ diff --git a/scripts/checksums b/scripts/checksums index 175038198..202d14c9b 100644 --- a/scripts/checksums +++ b/scripts/checksums @@ -1,2 +1,2 @@ -186d420b53e21c6daf8eabe980a5b90780c53bcd queue.h +db6784ff3917888db4d1dceaa0570d99ed40e762 queue.h 3337dbccc33eceedda78e36cc118d5a374838ec7 list.h From 68b64e3ea77393fbb006815b881f499c421e0b63 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Mon, 18 Mar 2024 19:55:25 +0800 Subject: [PATCH 04/11] Emphasize the use of American English When composing documentation, code comments, and other materials in English, please adhere to the American English (`en_US`) dialect. --- CONTRIBUTING.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 531846e62..48fa54da4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,6 +11,24 @@ Use your best judgment, and feel free to propose changes to this document in a p This project uses GitHub Issues to track ongoing development, discuss project plans, and keep track of bugs. Be sure to search for existing issues before you create another one. +Initially, it is advisable to create an issue on GitHub for bug reports, feature requests, +or substantial pull requests, as this offers a platform for discussion with both the community and project maintainers. + +Engaging in a conversation through a GitHub issue before making a contribution is crucial to ensure the acceptance of your work. +We aim to prevent situations where significant effort is expended on a pull request that might not align with the project's design principles. +For example, it might turn out that the feature you propose is more suited as an independent module that complements this project, +in which case we would recommend that direction. + +For minor corrections, such as typo fixes, small refactoring, or updates to documentation/comments, +filing an issue is not typically necessary. +What constitutes a "minor" fix involves discretion; however, examples include: +- Correcting spelling mistakes +- Minor code refactoring +- Updating or editing documentation and comments + +Nevertheless, there may be instances where, upon reviewing your pull requests, +we might request an issue to be filed to facilitate discussion on broader design considerations. + Visit our [Issues page on GitHub](https://github.com/sysprog21/lab0-c/issues) to search and submit. ## Coding Convention @@ -21,6 +39,10 @@ However, participation requires adherence to fundamental ground rules: While there is some flexibility in basic style, it is crucial to stick to the current coding standards. Complex algorithmic constructs without proper comments will not be accepted. * External pull requests should include thorough documentation in the pull request comments for consideration. +* When composing documentation, code comments, and other materials in English, + please adhere to the American English (`en_US`) dialect. + This variant should be considered the standard for all documentation efforts. + For instance, opt for "initialize" over "initialise" and "color" rather than "colour". Software requirement: [clang-format](https://clang.llvm.org/docs/ClangFormat.html) version 12 or later. From d48c56d1f88f0e7613e021f6685131b194529755 Mon Sep 17 00:00:00 2001 From: wuyihung Date: Mon, 18 Mar 2024 00:11:59 +0800 Subject: [PATCH 05/11] Improve string randomness Current implementation of function fill_rand_string() uses randombytes() to get random bytes and then gets modulus of 26. However, since the number of variations is 256, which is not exact division of 26, this causes the last four characters 'w', 'x', 'y', and 'z' appearing with less frequency than other characters. By testing, the entropy 4.699504 and arithmetic mean 109.3771 slightly deviates from the theoretical values log2(26)=4.700440 and 109.5, respectively. Regarding the samples and function to calculate the arithmetic mean, 150,000 samples were generated via the command "ih RAND" and these samples are used as argument to the "ent" command to calculate entropy and arithmetic mean. Here we expand buffer to 64-bit unsigned integer before getting random bytes. Calculating modulus on 64-bit unsigned integer gives more random result. After implementation, the entropy 4.700423 and arithmetic mean 109.5105 are improved to be closer to theoretical values. --- qtest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/qtest.c b/qtest.c index 0de4842ce..44857cf30 100644 --- a/qtest.c +++ b/qtest.c @@ -172,9 +172,11 @@ static void fill_rand_string(char *buf, size_t buf_size) while (len < MIN_RANDSTR_LEN) len = rand() % buf_size; - randombytes((uint8_t *) buf, len); + uint64_t randstr_buf_64[MAX_RANDSTR_LEN] = {0}; + randombytes((uint8_t *) randstr_buf_64, len * sizeof(uint64_t)); for (size_t n = 0; n < len; n++) - buf[n] = charset[buf[n] % (sizeof(charset) - 1)]; + buf[n] = charset[randstr_buf_64[n] % (sizeof(charset) - 1)]; + buf[len] = '\0'; } From 297bb617d40f6eb351176631cf6b89cb19a10c86 Mon Sep 17 00:00:00 2001 From: Aaron <162426224+aftuta85@users.noreply.github.com> Date: Thu, 28 Mar 2024 19:48:41 +0800 Subject: [PATCH 06/11] Remove duplicate argument check (#178) In do_size, argument checks are duplicated, with the second instance deemed redundant. --- qtest.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/qtest.c b/qtest.c index 44857cf30..a75039800 100644 --- a/qtest.c +++ b/qtest.c @@ -542,11 +542,6 @@ static bool do_size(int argc, char *argv[]) int reps = 1; bool ok = true; - if (argc != 1 && argc != 2) { - report(1, "%s needs 0-1 arguments", argv[0]); - return false; - } - if (argc == 2) { if (!get_int(argv[1], &reps)) report(1, "Invalid number of calls to size '%s'", argv[2]); From b0aa080e0ee61b745d8b6c30711496fff2f48bce Mon Sep 17 00:00:00 2001 From: yenslife <77geo5rge6@gmail.com> Date: Sun, 31 Mar 2024 03:59:25 +0800 Subject: [PATCH 07/11] Check if sorting implementation is stable Introduced node numbering in qtest.c to evaluate the stability of q_sort's sorting algorithm. When the algorithm encounters two nodes with the same value, it searches for the address of the node record in the nodes array. It then compares the found node to the current node (cur). If the found node is the same as the current node, it indicates that these two duplicate nodes have not been swapped in position after sorting. However, if the found node is cur->next, it means that the position of the nodes has been swapped. That is, the sorting implementation is unstable. The performance of the testing code was evaluated by measuring the elapsed time for q_sort's operation on different numbers of nodes with duplicate values. Node counts ranging from 1000 to 500,000 were examined. Specifically, for the 1000-node count, the elapsed time was recorded as 0.0279 seconds, and for the 500,000-node count, it was 61.44 seconds. For the 100,000-node count, the elapsed time was 2.45 seconds. The elapsed time showed a significant increase starting from the 100,000-node count, underscoring potential performance issues with larger datasets. This method relies on auxiliary data structures to track node pointers and their original order, avoiding alterations to the structure in queue.h. However, stability testing is limited to a maximum of 100,000 elements (MAX_NODES) to address potential performance concerns. --- qtest.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/qtest.c b/qtest.c index 44857cf30..8e2217d41 100644 --- a/qtest.c +++ b/qtest.c @@ -600,6 +600,23 @@ bool do_sort(int argc, char *argv[]) error_check(); set_noallocate_mode(true); + +/* If the number of elements is too large, it may take a long time to check the + * stability of the sort. So, MAX_NODES is used to limit the number of elements + * to check the stability of the sort. */ +#define MAX_NODES 100000 + struct list_head *nodes[MAX_NODES]; + unsigned no = 0; + if (current && current->size && current->size <= MAX_NODES) { + element_t *entry; + list_for_each_entry (entry, current->q, list) + nodes[no++] = &entry->list; + } else if (current && current->size > MAX_NODES) + report(1, + "Warning: Skip checking the stability of the sort because the " + "number of elements %d is too large, exceeds the limit %d.", + current->size, MAX_NODES); + if (current && exception_setup(true)) q_sort(current->q, descend); exception_cancel(); @@ -624,8 +641,32 @@ bool do_sort(int argc, char *argv[]) ok = false; break; } + /* Ensure the stability of the sort */ + if (current->size <= MAX_NODES && + !strcmp(item->value, next_item->value)) { + bool unstable = false; + for (unsigned i = 0; i < MAX_NODES; i++) { + if (nodes[i] == cur_l->next) { + unstable = true; + break; + } + if (nodes[i] == cur_l) { + break; + } + } + if (unstable) { + report( + 1, + "ERROR: Not stable sort. The duplicate strings \"%s\" " + "are not in the same order.", + item->value); + ok = false; + break; + } + } } } +#undef MAX_NODES q_show(3); return ok && !error_check(); From 9bda828f8c0dcd8a3a51def7fbfb02cffbade697 Mon Sep 17 00:00:00 2001 From: Wilson Liang <71745894+wilson20010327@users.noreply.github.com> Date: Mon, 1 Apr 2024 11:45:57 +0800 Subject: [PATCH 08/11] Prevent is_circular from infinite loop (#179) This commit refines the is_circular() to use slow and fast pointers instead of a single pointer iteration. The original approach struggled to detect mid-list loops without extra storage. The new method effectively identifies such loops, albeit with a minor increase in conditional checks, but without requiring additional storage. --- qtest.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/qtest.c b/qtest.c index a75039800..eae16dd85 100644 --- a/qtest.c +++ b/qtest.c @@ -875,17 +875,23 @@ static bool do_merge(int argc, char *argv[]) static bool is_circular() { struct list_head *cur = current->q->next; + struct list_head *fast = (cur) ? cur->next : NULL; while (cur != current->q) { - if (!cur) + if (!cur || !fast || !fast->next) + return false; + if (cur == fast) return false; cur = cur->next; + fast = fast->next->next; } cur = current->q->prev; + fast = (cur) ? cur->prev : NULL; while (cur != current->q) { - if (!cur) + if (!cur || !fast || !fast->prev) return false; cur = cur->prev; + fast = fast->prev->prev; } return true; } From 821eede0e766cbcca5309cd2b29cbf77e5383355 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sat, 13 Apr 2024 23:42:17 +0800 Subject: [PATCH 09/11] Prefer https for hyperlinks --- scripts/aspell-pws | 3 +++ scripts/pre-commit.hook | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/aspell-pws b/scripts/aspell-pws index cbe4db672..4b76ee400 100644 --- a/scripts/aspell-pws +++ b/scripts/aspell-pws @@ -292,3 +292,6 @@ adjtime perf uring Shannon +http +https +tcp diff --git a/scripts/pre-commit.hook b/scripts/pre-commit.hook index 4f45a7628..f5f0bda77 100755 --- a/scripts/pre-commit.hook +++ b/scripts/pre-commit.hook @@ -43,7 +43,7 @@ if [ -z "$($CPPCHECK --version | grep -E '^Cppcheck\s2')" ]; then if [ $CPPCHECK_VER -lt 90 ]; then echo "[!] cppcheck version must be at least 1.90." >&2 echo -e " Check 'Developer Info' for building Cppcheck from source:\n" \ - " http://cppcheck.sourceforge.net/devinfo/" >&2 + " https://cppcheck.sourceforge.net/devinfo/" >&2 exit 1 fi fi From 9ec084bbd5ce47de49831775b2c3ee97fe2dc222 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sun, 14 Apr 2024 00:48:35 +0800 Subject: [PATCH 10/11] Prefer https for hyperlinks --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 28b998b30..40fcd96b1 100644 --- a/README.md +++ b/README.md @@ -29,9 +29,9 @@ Some distros like Arch Linux won't install `aspell-en` with `aspell`, and you mu $ sudo pacman -S aspell-en ``` -Note: [Cppcheck](http://cppcheck.sourceforge.net/) version must be at least 1.90, otherwise +Note: [Cppcheck](https://cppcheck.sourceforge.net/) version must be at least 1.90, otherwise it might report errors with false positives. You can get its version by executing `$ cppcheck --version`. -Check [Developer Info](http://cppcheck.sourceforge.net/devinfo/) for building Cppcheck from source. +Check [Developer Info](https://cppcheck.sourceforge.net/devinfo/) for building Cppcheck from source. ### Integrate `clang-format` to `vim` If you want to run `clang-format` automatically after saving with vim, From bef955a9376040c994e2cddf632b5f3141ea2634 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Sun, 14 Apr 2024 00:50:15 +0800 Subject: [PATCH 11/11] Suppress Cppcheck warnings --- random.c | 30 ++++++++++++++---------------- report.c | 2 +- scripts/pre-commit.hook | 4 +++- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/random.c b/random.c index ff2b74fc0..4db1be761 100644 --- a/random.c +++ b/random.c @@ -90,7 +90,7 @@ static ssize_t getrandom(void *buf, size_t buflen, unsigned int flags) } #endif -static int randombytes_linux_randombytes_getrandom(void *buf, size_t n) +static int linux_getrandom(void *buf, size_t n) { size_t offset = 0; int ret; @@ -114,12 +114,12 @@ static int randombytes_linux_randombytes_getrandom(void *buf, size_t n) #if defined(__linux__) && !defined(SYS_getrandom) #if defined(__linux__) -static inline int randombytes_linux_read_entropy_ioctl(int device, int *entropy) +static inline int linux_read_entropy_ioctl(int device, int *entropy) { return ioctl(device, RNDGETENTCNT, entropy); } -static int randombytes_linux_read_entropy_proc(FILE *stream, int *entropy) +static int linux_read_entropy_proc(FILE *stream, int *entropy) { int retcode; do { @@ -131,7 +131,7 @@ static int randombytes_linux_read_entropy_proc(FILE *stream, int *entropy) return 0; } -static int randombytes_linux_wait_for_entropy(int device) +static int linux_wait_for_entropy(int device) { /* We will block on /dev/random, because any increase in the OS' entropy * level will unblock the request. I use poll here (as does libsodium), @@ -144,7 +144,7 @@ static int randombytes_linux_wait_for_entropy(int device) int entropy = 0; /* If the device has enough entropy already, we will want to return early */ - int retcode = randombytes_linux_read_entropy_ioctl(device, &entropy); + int retcode = linux_read_entropy_ioctl(device, &entropy); if (retcode != 0 && (errno == ENOTTY || errno == ENOSYS)) { /* The ioctl call on /dev/urandom has failed due to a * - ENOTTY (unsupported action), or @@ -184,11 +184,9 @@ static int randombytes_linux_wait_for_entropy(int device) continue; } else if (retcode == 1) { if (strategy == IOCTL) { - retcode = - randombytes_linux_read_entropy_ioctl(device, &entropy); + retcode = linux_read_entropy_ioctl(device, &entropy); } else if (strategy == PROC) { - retcode = - randombytes_linux_read_entropy_proc(proc_file, &entropy); + retcode = linux_read_entropy_proc(proc_file, &entropy); } else { return -1; /* Unreachable */ } @@ -220,7 +218,7 @@ static int randombytes_linux_wait_for_entropy(int device) } #endif /* defined(__linux__) */ -static int randombytes_linux_randombytes_urandom(void *buf, size_t n) +static int linux_urandom(void *buf, size_t n) { int fd; do { @@ -229,7 +227,7 @@ static int randombytes_linux_randombytes_urandom(void *buf, size_t n) if (fd == -1) return -1; #if defined(__linux__) - if (randombytes_linux_wait_for_entropy(fd) == -1) + if (linux_wait_for_entropy(fd) == -1) return -1; #endif @@ -259,7 +257,7 @@ static int randombytes_linux_randombytes_urandom(void *buf, size_t n) #include #endif #endif -static int randombytes_bsd_randombytes(void *buf, size_t n) +static int bsd_randombytes(void *buf, size_t n) { #if defined(MAC_OS_X_VERSION_10_15) && \ MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_15 @@ -279,16 +277,16 @@ int randombytes(uint8_t *buf, size_t n) #if defined(__linux__) || defined(__GNU__) #if defined(USE_GLIBC) /* Use getrandom system call */ - return randombytes_linux_randombytes_getrandom(buf, n); + return linux_getrandom(buf, n); #elif defined(SYS_getrandom) /* Use getrandom system call */ - return randombytes_linux_randombytes_getrandom(buf, n); + return linux_getrandom(buf, n); #else /* When we have enough entropy, we can read from /dev/urandom */ - return randombytes_linux_randombytes_urandom(buf, n); + return linux_urandom(buf, n); #endif #elif defined(BSD) - return randombytes_bsd_randombytes(buf, n); + return bsd_randombytes(buf, n); #else #error "randombytes(...) is not supported on this platform" #endif diff --git a/report.c b/report.c index 11b4de21b..1c841b802 100644 --- a/report.c +++ b/report.c @@ -61,7 +61,7 @@ void report_event(message_t msg, char *fmt, ...) "ERROR", "FATAL ERROR", }; - char *msg_name = msg_name_text[2]; + const char *msg_name = msg_name_text[2]; if (msg < N_MSG) msg_name = msg_name_text[msg]; int level = N_MSG - msg - 1; diff --git a/scripts/pre-commit.hook b/scripts/pre-commit.hook index f5f0bda77..88cb207b5 100755 --- a/scripts/pre-commit.hook +++ b/scripts/pre-commit.hook @@ -19,7 +19,9 @@ CPPCHECK_suppresses="--inline-suppr harness.c \ --suppress=returnDanglingLifetime:report.c \ --suppress=constParameterCallback:console.c \ --suppress=constParameterPointer:console.c \ ---suppress=checkLevelNormal:log2_lshift16.h" +--suppress=checkLevelNormal:log2_lshift16.h \ +--suppress=preprocessorErrorDirective:random.h \ +" CPPCHECK_OPTS="-I. --enable=all --error-exitcode=1 --force $CPPCHECK_suppresses $CPPCHECK_unmatched ." RETURN=0